fix(install): preserve LSP state before lockfile compare

[paul-ww]

Summary

This fixes no-op apm install lockfile churn for unchanged LSP dependencies.

The main lockfile builder already carries forward existing MCP state, local .apm state, and revision pin tags before semantic comparison, but it did not carry forward existing lsp_servers and lsp_configs. On a second unchanged install, that made the in-memory lockfile temporarily lose persisted LSP state, fail the semantic no-op check, and rewrite apm.lock.yaml even though only generated_at differed.

Closes #2078.

Changes

• carry forward existing lsp_servers before _write_if_changed(...)
• carry forward existing lsp_configs before _write_if_changed(...)
• add a focused regression test covering two unchanged installs with root lsp: config

Validation

• uv run --python 3.11 --with pytest python -m pytest tests/unit/install/test_mcp_lockfile_determinism.py
• uv run --python 3.11 --with pytest python -m pytest tests/unit/install/test_mcp_lockfile_determinism.py -k unchanged_lsp -vv
• uv run --python 3.11 --with ruff python -m ruff check src/apm_cli/install/phases/lockfile.py tests/unit/install/test_mcp_lockfile_determinism.py

[Copilot]

Pull request overview

Fixes apm install rewriting apm.lock.yaml on repeated no-op installs when only LSP-derived lockfile fields (lsp_servers, lsp_configs) would otherwise be temporarily dropped before the semantic no-op comparison.

Changes:

• Preserve existing lockfile lsp_servers / lsp_configs in LockfileBuilder before _write_if_changed(...) so semantic equivalence can succeed on unchanged installs.
• Add a regression test that runs two installs with unchanged root lsp: config and asserts byte-stable lockfile output (including generated_at stability after the second run).

Reviewed changes

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

File	Description
src/apm_cli/install/phases/lockfile.py	Carries forward existing LSP state (servers + configs) before the semantic lockfile comparison to avoid no-op churn.
tests/unit/install/test_mcp_lockfile_determinism.py	Adds a focused determinism regression test for unchanged LSP dependencies across repeated installs.

[paul-ww]

@microsoft-github-policy-service agree

[sergio-sisternes-epam]

APM Review Panel: ship_with_followups

Community-contributed fix restores LSP lockfile idempotency, closing a feature-parity gap with MCP carry-forward and eliminating spurious apm.lock.yaml rewrites on every install.

cc @paul-ww @danielmeppiel -- a fresh advisory pass is ready for your review.

Eight panelists reviewed; zero blocking findings. All specialists converged on the same assessment: this is a textbook pattern-consistent bug fix that mirrors the established MCP carry-forward exactly, touches no auth or supply-chain surface, and delivers a real user-visible improvement (no more phantom git diffs on every apm install for LSP users). The panel's signal-to-noise ratio is unusually clean -- five panelists found only nits or nothing, and the three recommended findings all point in the same direction: document the fix and close the remaining test-coverage gap.

The CHANGELOG finding was raised independently by both doc-writer and oss-growth-hacker with nearly identical wording. Crediting @paul-ww by handle and linking #2079 satisfies Keep a Changelog conventions and is the cheapest contributor-retention lever available. This should be folded into the PR before merge -- it's a one-line addition to the empty [Unreleased] block.

The test-coverage-expert's missing-positive-change-test finding carries real weight on a portability-by-manifest surface. Without test_changed_lsp_dependencies_update_lockfile, a future bug silently dropping changed LSP configs would go undetected. That said, the missing test is for a code path not touched by this PR (pre-existing coverage debt on the LSP integrator's update path). File a follow-up issue; blocking this fix on it would over-index on coverage purity at the cost of shipping a real user-facing improvement today.

Aligned with: Portable by manifest (lockfile idempotency is load-bearing for reproducible environments), Pragmatic as npm (users expect install to be a no-op when nothing changed), OSS community driven (community contributor fixing a feature-parity gap).

Growth signal. @paul-ww independently discovered and fixed an LSP/MCP parity gap -- early evidence that APM's LSP surface has real adoption beyond the core MCP use case. Worth a callout in the next release announcement.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	2	Textbook pattern-consistent bug fix; LSP carry-forward mirrors MCP exactly. No architectural concerns. Ship.
CLI Logging Expert	0	0	1	New verbose_detail log lines are pattern-consistent with existing MCP/local-state carry-forward messages; no concerns from CLI output lens.
DevX UX Expert	0	0	1	Fixes a real idempotency violation visible to every LSP user; no CLI surface change, no new edge cases. Ship.
Supply Chain Security	0	0	0	No supply-chain surface touched. LSP carry-forward mirrors established MCP pattern; locally-sourced config only, overwritten same run. Ship.
OSS Growth Hacker	0	1	1	Community-contributed LSP parity fix removes real friction; deserves CHANGELOG credit and a release-note beat.
Doc Writer	0	1	0	CHANGELOG [Unreleased] is empty -- this user-visible lockfile idempotency fix warrants a ### Fixed entry per established APM conventions.
Test Coverage Expert	0	1	0	New LSP carry-forward regression test is solid integration-with-fixtures; missing symmetric positive-change test (LSP deps changed -> lockfile rewritten).
Performance Expert	0	0	2	Net positive: deepcopy cost is sub-microsecond on tiny LSP dicts; the real win is skipping unnecessary lockfile writes on no-op installs (~5-15ms saved).

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

Top 3 follow-ups

1. [Test Coverage Expert] Add test_changed_lsp_dependencies_update_lockfile -- the positive-change counterpart to the new regression test. -- Evidence outcome: missing on portability-by-manifest surface. Without it, a future bug silently dropping changed LSP configs would go undetected. File as a follow-up issue; don't block this PR.
2. [Doc Writer] Add CHANGELOG [Unreleased] ### Fixed entry for the LSP lockfile idempotency fix, crediting @paul-ww. -- User-visible fix with no CHANGELOG entry violates Keep a Changelog conventions. Same class as Fix: make --skill deployment additive, not just persistence (#1786 follow-up) #1955 and [BUG] apm install deploys skill-bundle files verbatim with CRLF, causing content-integrity hash drift in CI #1952, both of which got entries. Fold into this PR before merge.
3. [Python Architect] Track duplication across _preserve_existing_* methods; extract a helper if a fifth carry-forward arrives. -- Four near-identical methods is tolerable; five is not. No action now, but worth a tech-debt issue.

Architecture

classDiagram
    direction LR
    class LockfileBuilder {
        <<Orchestrator>>
        +build_and_save() None
        -_preserve_existing_mcp_state(lockfile) None
        -_preserve_existing_lsp_state(lockfile) None
        -_preserve_existing_local_state(lockfile) None
        -_preserve_existing_revision_pin_tags(lockfile) None
        -_write_if_changed(lockfile, path, LF) None
    }
    class InstallContext {
        <<ValueObject>>
        +existing_lockfile LockFile
        +installed_packages list
        +logger CommandLogger
    }
    class LockFile {
        <<DataClass>>
        +lsp_servers list~str~
        +lsp_configs dict
        +mcp_servers list~str~
        +mcp_configs dict
        +is_semantically_equivalent(other) bool
        +save(path) None
    }
    class MCPIntegrator {
        <<Reconciler>>
        +update_lockfile(servers, path, configs) None
    }
    class LSPIntegrator {
        <<Reconciler>>
        +update_lockfile(servers, path, configs) None
    }
    LockfileBuilder *-- InstallContext : reads ctx
    LockfileBuilder ..> LockFile : builds and saves
    MCPIntegrator ..> LockFile : reconciles mcp fields
    LSPIntegrator ..> LockFile : reconciles lsp fields
    class LockfileBuilder:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm install CLI entry"] --> B["LockfileBuilder.build_and_save()"]
    B --> C["_attach_deployed_files()"]
    C --> D["_merge_existing()"]
    D --> E["_maybe_merge_partial()"]
    E --> F["_preserve_existing_mcp_state()"]
    F --> G["_preserve_existing_lsp_state() -- THIS PR"]
    G --> H["_preserve_existing_local_state()"]
    H --> I["_preserve_existing_revision_pin_tags()"]
    I --> J{"_write_if_changed() is_semantically_equivalent?"}
    J -->|equivalent| K["Skip write -- no lockfile churn"]
    J -->|different| L["lockfile.save()"]
    L --> M["_sync_cache_pin_markers()"]
    K --> M
    M --> N["LSPIntegrator.update_lockfile() -- reconciles lsp fields"]
    N --> O["MCPIntegrator.update_lockfile() -- reconciles mcp fields"]
    style G fill:#fff3b0,stroke:#d47600

Loading

Recommendation

Merge after folding a one-line CHANGELOG entry crediting @paul-ww. The missing positive-change test is real coverage debt but pre-existing -- file it as a follow-up issue rather than blocking this fix. All panelists agree the code is pattern-consistent, safe, and user-visible. Ship it, thank the contributor, and track the LSP test gap in a separate issue.

---

Full per-persona findings

Python Architect

• [nit] Four near-identical _preserve_existing_* methods share the same guard-copy-log skeleton at src/apm_cli/install/phases/lockfile.py:322
  MCP and LSP carry-forward are now character-for-character twins. At 4 methods the duplication is tolerable; if a fifth arrives, extracting a helper would be warranted. Not actionable on this PR.

• [nit] Verbose log says 'unchanged' but the carry-forward is unconditional at src/apm_cli/install/phases/lockfile.py:329
  MCP twin has the same wording so this is inherited, not introduced. Pre-existing cosmetic issue.

CLI Logging Expert

• [nit] LSP carry-forward message says 'unchanged' before reconciliation has run at src/apm_cli/install/phases/lockfile.py:329
  MCP sibling has the same wording -- inherited, not introduced.

DevX UX Expert

• [nit] Verbose log line says 'unchanged' but the method unconditionally overwrites the field at src/apm_cli/install/phases/lockfile.py:328
  Pre-existing phrasing inherited from MCP twin.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

• [recommended] Add an [Unreleased] CHANGELOG entry crediting the community contributor at CHANGELOG.md:8
  Fix is user-visible; crediting contributors in CHANGELOG is the cheapest retention lever for OSS.
  Suggested: ### Fixed - apm install no longer rewrites apm.lock.yaml on every run when LSP dependencies are unchanged, matching the existing MCP idempotency guarantee. (by @paul-ww, #2079)

• [nit] Test name is excellent -- consider a one-line comment linking to the MCP sibling at tests/unit/install/test_mcp_lockfile_determinism.py:302

Doc Writer

• [recommended] Missing CHANGELOG entry for user-visible LSP lockfile idempotency fix at CHANGELOG.md:8
  APM follows Keep a Changelog strictly. [Unreleased] block is empty. Same class as Fix: make --skill deployment additive, not just persistence (#1786 follow-up) #1955 and [BUG] apm install deploys skill-bundle files verbatim with CRLF, causing content-integrity hash drift in CI #1952, both of which got entries.

Test Coverage Expert

• [recommended] Missing test_changed_lsp_dependencies_update_lockfile -- positive-change counterpart to the new regression test
  MCP surface has a symmetric pair; LSP surface added only the unchanged half. Pre-existing coverage debt -- suitable for a follow-up issue.
  Proof (missing at integration-with-fixtures): tests/unit/install/test_mcp_lockfile_determinism.py::test_changed_lsp_dependencies_update_lockfile -- proves: When a user changes LSP dependencies in apm.yml, apm install rewrites the lockfile with the new LSP state. [devx, portability-by-manifest]

Performance Expert

• [nit] deepcopy is correct; consistency with MCP sibling wins. No change needed. src/apm_cli/install/phases/lockfile.py:326

• [nit] Consider hoisting the verbose_detail guard to avoid string formatting on non-verbose runs. Once-per-install, <1us. Not worth changing on this PR. src/apm_cli/install/phases/lockfile.py:328

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

[sergio-sisternes-epam]

@paul-ww please update the CHANGELOG following the conventions, including the credit to yourself :)

[danielmeppiel]

APM Review Panel: needs_rework

Preserves LSP state before lockfile semantic comparison, completing the no-op-install correctness guarantee established for MCP state.

cc @paul-ww @danielmeppiel -- a fresh advisory pass is ready for your review.

The panel converges on the implementation: it mirrors the existing MCP carry-forward pattern, introduces no output, security, documentation, or performance regression, and its unit test passes. The substantive gap is evidence at the real install boundary: no test under tests/integration/ runs repeated installs with unchanged LSP dependencies and proves apm.lock.yaml remains byte-identical.

Aligned with: portability by manifest through deterministic lockfiles; pragmatic package-manager behavior through idempotent installs.

Panel summary

Persona	B	R	N	Takeaway
Python architect	0	1	1	Implementation is sound; integration evidence is missing.
CLI logging expert	0	0	0	Verbose output follows the existing MCP pattern.
DevX UX expert	0	0	0	No CLI surface regression.
Supply-chain security expert	0	0	0	No new integrity or credential surface.
OSS growth hacker	0	0	0	Internal reliability fix; no conversion surface touched.
Test coverage expert	0	1	1	Unit proof passes, but lockfile determinism needs integration-with-fixtures evidence.
Performance expert	0	0	0	Tiny in-memory copies have no measurable impact.

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

Top 2 follow-ups

1. [Test coverage expert] Add an integration-with-fixtures regression that runs repeated installs with unchanged LSP dependencies and asserts byte-identical apm.lock.yaml output.
2. [Python architect] Consider extracting a generic carry-forward helper if another state family adopts this pattern; this is low-priority cleanup.

Recommendation

Add the empirical integration regression and mutation-break it, then re-run the panel. The helper-extraction nit can wait because it introduces a separate abstraction theme.

---

Full per-persona findings

Python architect

• [recommended] Missing integration-tier repeated-install regression for LSP lockfile determinism at tests/integration/.
• [nit] A generic state carry-forward helper could reduce future duplication.

CLI logging expert

No findings.

DevX UX expert

No findings.

Supply-chain security expert

No findings.

OSS growth hacker

No findings.

Auth expert -- inactive

No authentication surface is touched.

Doc writer -- inactive

The internal no-op behavior fix creates no documentation drift.

Test coverage expert

• [recommended] Unit coverage passes, but no integration-with-fixtures test proves the full repeated-install promise.
• [nit] The unit test is valid narrow evidence for the carry-forward helper.

Performance expert

No findings.

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

[danielmeppiel]

APM Review Panel: ship_now

Lock-state carry-forward lands with byte-stability proof and a mutation-break regression trap.

cc @paul-ww @danielmeppiel -- the converged advisory pass is ready for review.

All specialists aligned. The LSP lock-state carry-forward mirrors the established MCP pattern, and the new integration test exercises the real CLI install path twice with unchanged LSP dependencies. Removing _preserve_existing_lsp_state makes that test fail, proving the guard is load-bearing. CI is green on the exact final SHA.

Aligned with: portability by manifest through deterministic lockfiles; pragmatic package-manager behavior through idempotent installs.

Panel summary

Persona	B	R	N	Takeaway
Python architect	0	0	1	Symmetric carry-forward pattern is correct and bounded.
CLI logging expert	0	0	0	Verbose output follows the established pattern.
DevX UX expert	0	0	0	The no-op install contract is restored.
Supply-chain security expert	0	0	0	No new trust or integrity surface.
OSS growth hacker	0	0	0	No growth surface touched.
Test coverage expert	0	0	0	Integration regression passes and mutation-breaks.
Performance expert	0	0	0	Small in-memory copies have negligible cost.

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

Folded in this run

• (panel) Added tests/integration/test_install_lsp_lockfile_determinism.py, which invokes apm install twice and proves unchanged LSP dependencies leave apm.lock.yaml byte-identical -- resolved in 46c4f0c45546378cefbe3c7e13778c399cd7b73d.

Regression-trap evidence (mutation-break gate)

• tests/integration/test_install_lsp_lockfile_determinism.py::test_repeated_install_with_unchanged_lsp_keeps_lockfile_bytes -- removed the _preserve_existing_lsp_state(lockfile) call; the test failed because generated_at changed; guard restored.

Copilot signals reviewed

Copilot reviewed the changed files and produced no inline findings across two fetch rounds.

Lint contract

The full CI lint mirror passed before push: ruff check, ruff format check, YAML I/O guard, file-length guard, relative-path guard, pylint R0801, and auth-signal boundary check.

CI

All checks passed on exact head SHA 46c4f0c45546378cefbe3c7e13778c399cd7b73d, including both test shards, integration collection, lint, CodeQL, binary smoke, spec conformance, NOTICE drift, and merge gate: https://github.com/microsoft/apm/actions/runs/29056037868

Mergeability status

PR	head SHA	CEO stance	iters	folds	defers	Copilot rounds	CI	mergeable	mergeStateStatus	notes
#2079	46c4f0c	ship_now	2	1	0	2	green	MERGEABLE	BLOCKED	awaiting required review

Recommendation

All in-scope findings are resolved. Final panel stance: ship_now.

---

Full per-persona findings

No substantive findings remain. Python architect noted that a generic state carry-forward abstraction could be considered in a separate refactor if the pattern expands; it is not part of this narrowly scoped bug fix.

_{This panel is advisory. It does not block merge.}