fix: pass scope-resolved lockfile path to MCPIntegrator.update_lockfile at --global scope by sergio-sisternes-epam · Pull Request #1236 · microsoft/apm

sergio-sisternes-epam · 2026-05-10T10:41:44Z

Description

Fix MCPIntegrator.update_lockfile() to use the scope-resolved lockfile path when operating at --global scope. Currently, update_lockfile() defaults to Path.cwd(), causing MCP server audit entries to be written to the project-local lockfile instead of ~/.apm/apm.lock.yaml.

Fixes #794

Type of change

Bug fix
New feature
Documentation
Maintenance / refactor

Testing

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

…le at --global scope Work in progress — signals intent to fix #794. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Fixes a --global scope bug where MCPIntegrator.update_lockfile() could default to Path.cwd() and persist MCP audit/config entries into the project lockfile instead of the user-scope lockfile.

Changes:

Pass the already scope-resolved lockfile path (_lock_path) into MCPIntegrator.update_lockfile() from the main install flow.
In the --mcp install path, reuse a computed lockfile path for both reading and updating the lockfile.
Add unit regression tests that statically assert callers pass a lockfile path into update_lockfile().

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
tests/unit/test_global_mcp_scope.py	Adds regression tests intended to prevent `update_lockfile()` from being called without an explicit lockfile path.
src/apm_cli/install/mcp/command.py	Computes and reuses a lockfile path, then passes it into `update_lockfile()` to avoid defaulting to CWD.
src/apm_cli/commands/install.py	Passes the scope-resolved `_lock_path` into `update_lockfile()` so `--global` writes to the correct lockfile.

github-actions · 2026-05-10T11:10:00Z

APM Review Panel: `ship_with_followups`

PR #1236 correctly routes --global MCP lockfile writes; ship with CHANGELOG entry and integration-level regression trap as non-blocking followups.

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

The fix is structurally sound. Python Architect confirms it mirrors the established uninstall.py pattern, all four call sites now supply lock_path explicitly, and the signature change is internally consistent. DevX UX Expert and Supply-Chain Security Expert independently surface the same latent trap: the Path.cwd() fallback default on update_lockfile() remains in place, meaning any future call site added without an explicit argument will silently misdirect writes with no warning. This is the original root cause of #794, not merely a symptom. The panel converges on recommending the default be hardened (required arg or RuntimeError on None) -- I endorse this as a followup, not a blocker, because all current call sites are already correct post-PR.

The test-coverage gap is the most consequential open item. Test-Coverage Expert returned an evidence block with outcome: missing on an integration-with-fixtures tier for a governed-by-policy surface (lockfile determinism at --global scope). The two new AST-static tests guard against re-introducing the missing-arg form but cannot catch the wrong path being constructed, the argument being ignored, or a keyword-arg refactor with a wrong default. The integration test suggested -- assert MCP entry lands in fake_home/.apm/apm.lock.yaml and NOT in cwd/apm.lock.yaml -- is the exact regression trap for #794 and should land in the next sprint, not this PR.

OSS Growth Hacker and Doc Writer converge on a CHANGELOG gap: the [Unreleased] Fixed section has no entry for this fix. Issue #794 was a publicly tracked known gap from feature #638; users who churned on the broken --global MCP install deserve a clear upgrade signal. A one-line Fixed entry is a five-minute addition and a direct conversion lever.

Dissent. No material disagreement between panelists. DevX UX Expert and Python Architect both flag AST test fragility (nit tier); both are superseded by Test-Coverage Expert's governed-by-policy evidence block, which carries higher weight. CLI Logging Expert and DevX UX Expert both flag the silent-skip on missing global lockfile -- DevX frames it as a user promise gap, CLI Logging frames it as a --verbose diagnostic gap; these are the same finding from two angles and are surfaced once in followups. Supply-Chain Security Expert raises a TOCTOU double-read between run_mcp_install and update_lockfile's internal LockFile.read(); this is pre-existing and inherits nit weight only.

Aligned with: Secure by default (MCP audit entries now route correctly; residual Path.cwd() fallback is a latent risk for future call sites), Governed by policy (correct routing restored but no integration-tier automated guardrail exists yet), OSS community-driven (silent fix without CHANGELOG entry mutes the trust-building signal), Pragmatic as npm (--global now mutates the user-scope state file as users expect).

Growth signal. The #638 -> #794 -> #1236 arc is a three-PR transparency story: feature shipped, gap acknowledged publicly, gap closed. That arc converts watchers into contributors when it is visible. A single CHANGELOG line and a sentence in the next release post ('We tracked and closed every known gap from the initial --global MCP launch') turns a silent bug fix into a community trust signal. OSS Growth Hacker also flags a FAQ entry on 'Where does APM write global MCP entries?' as a long-tail deflection play worth adding to the doc backlog.

Panel summary

Persona	R	N	Takeaway
Python Architect	1	1	Fix is correct and consistent with uninstall.py pattern; signature matches, _lock_path is unconditionally set; AST test is a weak substitute for a behavioral mock test.
CLI Logging Expert	1	1	Fix is correct but update_lockfile() still emits zero success-path logging; --verbose users cannot confirm which lockfile was written at global scope.
DevX UX Expert	1	2	Bug fix correctly aligns --global lockfile writes with user mental model; silent fallback default on update_lockfile is a latent trap worth hardening.
Supply Chain Security Expert	0	2	No new supply-chain vulnerabilities introduced; path resolution is scope-guarded via get_apm_dir(); one pre-existing silent-drop nit surfaced.
OSS Growth Hacker	1	1	Fix completes the --global MCP install story first shipped in #638; no [Unreleased] CHANGELOG entry means the community never learns the known gap (#794) is closed.
Doc Writer	1	0	CHANGELOG.md has no entry for the --global scope lockfile path fix; user-observable bug fix requires a Fixed entry in [Unreleased].
Test Coverage Expert	1	0	AST-static regression traps added for #794 but no integration test exercises MCP update_lockfile at --global scope at runtime; lockfile-determinism floor unmet for the MCP path.

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

Top 5 follow-ups

[Test Coverage Expert] Add integration test asserting apm install --global <mcp-server> writes entry to fake_home/.apm/apm.lock.yaml and NOT to cwd/apm.lock.yaml -- outcome: missing on a governed-by-policy surface (lockfile determinism). AST-static tests cannot catch wrong path construction, ignored arguments, or keyword-arg refactors. This is the only test that would have caught Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794 and will catch a recurrence.
[OSS Growth Hacker] Add [Unreleased] > Fixed CHANGELOG entry: "apm install --global now writes MCP server audit entries to ~/.apm/apm.lock.yaml instead of the project-local lockfile. (closes Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794)" -- Users who churned on the --global MCP install bug need an upgrade signal. Doc Writer independently converges. Must land before the next release cut.
[DevX UX Expert] Remove Path.cwd() default from update_lockfile(); make lock_path required or raise RuntimeError when None -- Path.cwd() is the original root cause of Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794. All callers supply the arg post-PR, so making it required is a zero-cost hardening. Future call sites added without an explicit arg will fail loudly instead of silently misdirecting writes.
[CLI Logging Expert] Add _log.debug('Updated MCP server entries in lockfile at %s', lock_path) after lockfile.save() in update_lockfile() -- The entire motivation of Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794 was silent misdirection. The fix corrects routing but leaves the operation opaque at --verbose. One debug line closes the observability gap and aids future debugging.
[DevX UX Expert] Emit a warning (not a silent return) when the global lockfile does not exist at install time -- A first-time --global user who has never run apm init --global will get MCP servers installed with no lockfile entry and no message explaining why audit/drift detection will not work. Silent discard on a governed-by-policy surface is a user promise violation.

Architecture

classDiagram
    direction LR

    class MCPIntegrator {
      <<Integrator>>
      +install(servers, apm_dir, ...) None
      +update_lockfile(names, lock_path, mcp_configs) None
    }

    class LockFile {
      <<ValueObject>>
      +mcp_servers list
      +mcp_configs dict
      +read(path) LockFile
      +save(path) None
    }

    class InstallContext {
      <<ValueObject>>
      +apm_dir Path
      +scope InstallScope
    }

    class InstallScope {
      <<Enum>>
      USER
      PROJECT
      GLOBAL
    }

    class get_lockfile_path {
      <<Pure>>
      +__call__(project_root) Path
    }

    class _install_apm_packages {
      <<IOBoundary>>
      +__call__(ctx, outcome) None
    }

    class run_mcp_install {
      <<IOBoundary>>
      +__call__(apm_dir, ...) None
    }

    class MCPIntegrator:::touched
    class _install_apm_packages:::touched
    class run_mcp_install:::touched

    _install_apm_packages ..> InstallContext : reads ctx.apm_dir
    _install_apm_packages ..> get_lockfile_path : derives _lock_path
    _install_apm_packages ..> MCPIntegrator : calls update_lockfile
    run_mcp_install ..> get_lockfile_path : derives _mcp_lock_path
    run_mcp_install ..> MCPIntegrator : calls update_lockfile
    MCPIntegrator ..> LockFile : reads and writes
    MCPIntegrator ..> get_lockfile_path : fallback when lock_path=None
    InstallContext *-- InstallScope : scope

    note for MCPIntegrator "lock_path=None fallback (CWD) is now dead for all in-tree callers post this PR"

    classDef touched fill:#fff3b0,stroke:#d47600

flowchart TD
    A(["apm install --global / apm mcp install --global"]) --> B

    subgraph install.py
        B["_install_apm_packages(ctx, outcome)"]
        B --> C["[I/O] _lock_path = get_lockfile_path(ctx.apm_dir)\n~/.apm/apm.lock.yaml when scope=GLOBAL"]
        C --> D{MCP branch?}
        D -- "new MCP servers" --> E["[FS] MCPIntegrator.update_lockfile(\n  new_mcp_servers, _lock_path, ...)"]
        D -- "no MCP deps, old servers exist" --> F["[FS] MCPIntegrator.update_lockfile(\n  set(), _lock_path, mcp_configs={})"]
        D -- "--only=apm restore" --> G["[FS] MCPIntegrator.update_lockfile(\n  old_mcp_servers, _lock_path, ...)"]
    end

    subgraph command.py
        H["run_mcp_install(apm_dir, ...)"] --> I["[I/O] _mcp_lock_path = get_lockfile_path(apm_dir)"]
        I --> J["[FS] LockFile.read(_mcp_lock_path)"]
        J --> K["MCPIntegrator.install(...)"]
        K --> L["[FS] MCPIntegrator.update_lockfile(\n  merged_names, _mcp_lock_path, ...)"]
    end

    subgraph mcp_integrator.py
        M["MCPIntegrator.update_lockfile(names, lock_path, ...)"] --> N{lock_path is None?}
        N -- "yes (fallback, unreachable in-tree)" --> O["[FS] lock_path = get_lockfile_path(Path.cwd())"]
        N -- "no (all in-tree callers)" --> P
        O --> P["[I/O] LockFile.read(lock_path)"]
        P --> Q["[FS] lockfile.save(lock_path)"]
    end

    E --> M
    F --> M
    G --> M
    L --> M

    style E fill:#fff3b0,stroke:#d47600
    style F fill:#fff3b0,stroke:#d47600
    style G fill:#fff3b0,stroke:#d47600
    style L fill:#fff3b0,stroke:#d47600
    style I fill:#fff3b0,stroke:#d47600

Recommendation

The fix is correct, pattern-consistent, and closes a publicly tracked known gap. No panelist found a blocking defect in the changed lines. The two open items that matter most -- the integration regression trap and the CHANGELOG entry -- are mechanical and low-risk; neither justifies holding the fix from users who are actively affected by #794. Merge with explicit tracking of the five followups above, and treat the CHANGELOG entry as must-land before the next release tag is cut.

Full per-persona findings

Python Architect

[recommended] AST regression test does not verify the path value passed at --global scope at tests/unit/test_global_mcp_scope.py
The test uses static AST inspection to assert >= 2 positional args exist in each update_lockfile call. This guards against re-introducing the missing-arg form but does not assert that the argument is the scope-resolved path when --global is active. A developer could pass Path.cwd() explicitly and the AST test would still pass.
Suggested: Add a parametrized unit test that patches get_lockfile_path to return a known sentinel path, invokes the relevant branch with scope=GLOBAL, and asserts MCPIntegrator.update_lockfile.call_args[0][1] == sentinel_path.
[nit] CWD fallback in update_lockfile is now an unreachable dead branch for all in-tree callers at src/apm_cli/integration/mcp_integrator.py:771
All four call sites now supply lock_path explicitly. The lock_path=None fallback is effectively dead code for production flows. Worth a comment so future readers do not assume the default is ever exercised in normal operation.
Suggested: Add comment: # Fallback: should not be reached by in-tree callers; all supply lock_path explicitly.

CLI Logging Expert

[recommended] update_lockfile() has no success-path log -- written path invisible at --verbose at src/apm_cli/integration/mcp_integrator.py:782
After lockfile.save(lock_path) there is no log statement at any level. The entire motivation of this PR is that MCP audit entries were silently routed to the wrong file. The fix corrects the routing, but the operation remains opaque at --verbose.
Suggested: After lockfile.save(lock_path) add: _log.debug('Updated MCP server entries in lockfile at %s', lock_path)
[nit] Silent return when lockfile does not exist gives no diagnostic at global scope at src/apm_cli/integration/mcp_integrator.py:773
If ~/.apm/apm.lock.yaml does not exist yet (fresh global install), update_lockfile() returns silently. At --verbose a single debug log would help distinguish 'path resolved correctly but file absent' from 'path was wrong'.
Suggested: _log.debug('Skipping MCP lockfile update -- %s does not exist', lock_path); return

DevX UX Expert

[recommended] update_lockfile default Path.cwd() fallback remains a silent misdirection trap at src/apm_cli/integration/mcp_integrator.py:772
MCPIntegrator.update_lockfile() still defaults to Path.cwd() when lock_path is None. This PR fixes call sites, but the silent fallback is the original root cause of Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794. Any new call site added without the explicit argument will silently write to the wrong lockfile at --global scope with no error, no warning, and no user-visible signal.
Suggested: Remove the Path.cwd() default. Make lock_path a required positional argument, OR raise RuntimeError when lock_path is None. All callers already pass it after this PR.
[nit] AST-based regression tests are structurally fragile at tests/unit/test_global_mcp_scope.py
Tests assert >= 2 positional args, but would break if refactored to keyword-only args (a valid and more readable form) -- producing a false positive. A behavioral mock test would be more resilient.
[nit] No user-visible feedback when --global lockfile write is skipped at src/apm_cli/integration/mcp_integrator.py:773
update_lockfile silently returns when the lockfile does not exist. At --global scope a first-time user who has never run apm init --global will get MCP servers installed but no lockfile entry and no message explaining why audit/drift detection will not work.

Supply Chain Security Expert

[nit] Silent drop when global lockfile does not yet exist at src/apm_cli/integration/mcp_integrator.py:775
update_lockfile() returns early without error if lock_path does not exist. For a first-time --global install, ~/.apm/apm.lock.yaml may not exist yet, causing all MCP audit entries to be silently discarded rather than initialising the file. Pre-existing issue now exercised on a new code path.
[nit] Double-read TOCTOU between _existing_lock and update_lockfile's internal read at src/apm_cli/install/mcp/command.py:119
run_mcp_install reads the lockfile at line 120, but MCPIntegrator.update_lockfile() performs its own independent LockFile.read() internally. The two reads are not atomic; a concurrent process modifying ~/.apm/apm.lock.yaml between the two reads could cause divergence silently. Pre-existing design pattern, not introduced by this PR.

OSS Growth Hacker

[recommended] No CHANGELOG entry for the --global lockfile path fix at CHANGELOG.md
The global MCP install feature (Fix apm install --global skipping MCP server installation #638) shipped with a documented known gap tracked in Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794. This PR closes that gap, but without a [Unreleased] Fixed entry, existing users who hit Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794 have no signal that upgrading resolves their problem.
Suggested: Add to [Unreleased] > Fixed: "apm install --global now writes MCP server audit entries to ~/.apm/apm.lock.yaml instead of the project-local lockfile. (closes Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794)"
[nit] The Fix apm install --global skipping MCP server installation #638 -> Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794 -> fix: pass scope-resolved lockfile path to MCPIntegrator.update_lockfile at --global scope #1236 fix arc is a trust-building story worth a sentence in the next release post.
Three-PR arc shows transparent gap ownership and follow-through -- exactly the trust signal that converts watchers into contributors.

Doc Writer

[recommended] Missing CHANGELOG entry for --global lockfile path bug fix at CHANGELOG.md
PR fix: pass scope-resolved lockfile path to MCPIntegrator.update_lockfile at --global scope #1236 fixes a user-observable bug: apm install --global was silently writing MCP state to the project-scope lockfile instead of the user-scope one. The [Unreleased] > Fixed section in CHANGELOG.md has no corresponding entry.
Suggested: Add: "apm install --global now writes MCP server entries to the user-scope lockfile (~/.apm/apm.lock.yaml) instead of the project-scope lockfile. (fix: pass scope-resolved lockfile path to MCPIntegrator.update_lockfile at --global scope #1236)"

Test Coverage Expert

[recommended] No integration test verifies that an MCP server install with --global writes its audit entry to ~/.apm/apm.lock.yaml rather than cwd/apm.lock.yaml at tests/integration/
The two new tests are AST-based static analysis (tier=static). Grepped tests/integration/ for mcp+global patterns -- zero matches. tests/integration/test_global_scope_e2e.py::test_lockfile_placed_under_user_dir covers lockfile placement for local-bundle but does NOT invoke the MCP code path. Per the tier-floor matrix, lockfile determinism requires integration-with-fixtures coverage.
Proof (missing at integration-with-fixtures): tests/integration/test_global_mcp_lockfile_e2e.py::test_global_mcp_install_writes_lockfile_to_user_dir -- proves: apm install --global <mcp-server> writes the MCP audit entry to ~/.apm/apm.lock.yaml and NOT to cwd/apm.lock.yaml (the exact regression from Bug: MCPIntegrator.update_lockfile uses Path.cwd() at --global scope, writing MCP audit entries to the wrong lockfile #794) [devx, governed-by-policy]
Suggested: Add test to tests/integration/test_global_scope_e2e.py (or new tests/integration/test_global_mcp_lockfile_e2e.py) that installs a minimal fixture MCP server with --global, then asserts: (1) fake_home/.apm/apm.lock.yaml contains the MCP server entry, (2) work_dir/apm.lock.yaml does NOT contain the MCP server entry.

Auth Expert -- inactive

Changed files address lockfile path resolution for MCP server scope and do not touch authentication, token management, credential resolution, host classification, or any AuthResolver surface.

_{This panel is advisory. It does not block merge. Re-apply the

panel-review label after addressing feedback to re-run.}

Generated by PR Review Panel for issue #1236 · ● 2.3M · ◷

At --global scope, MCPIntegrator.update_lockfile() was falling back to Path.cwd() because callers in the install pipeline did not forward the scope-resolved lockfile path. This caused MCP server audit entries to be written to the project-local lockfile instead of ~/.apm/apm.lock.yaml. Fix: pass the already-computed _lock_path (from ctx.apm_dir) to all update_lockfile() call sites in install.py and mcp/command.py. The uninstall engine was already correct. Regression tests use AST inspection to verify every update_lockfile call in both modules passes lock_path positionally, preventing silent re-introduction of the bug. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

fix: pass scope-resolved lockfile path to MCPIntegrator.update_lockfi…

27c4def

…le at --global scope Work in progress — signals intent to fix #794. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

sergio-sisternes-epam marked this pull request as ready for review May 10, 2026 10:57

sergio-sisternes-epam requested a review from danielmeppiel as a code owner May 10, 2026 10:57

Copilot AI review requested due to automatic review settings May 10, 2026 10:57

sergio-sisternes-epam added the panel-review Trigger the apm-review-panel gh-aw workflow label May 10, 2026

Copilot started reviewing on behalf of sergio-sisternes-epam May 10, 2026 10:58 View session

Copilot AI reviewed May 10, 2026

View reviewed changes

Comment thread tests/unit/test_global_mcp_scope.py

Comment thread tests/unit/test_global_mcp_scope.py

Comment thread tests/unit/test_global_mcp_scope.py

Comment thread tests/unit/test_global_mcp_scope.py

github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 10, 2026

sergio-sisternes-epam force-pushed the fix/794-global-lockfile-path branch from a159347 to 833bbac Compare May 10, 2026 11:22

danielmeppiel approved these changes May 10, 2026

View reviewed changes

danielmeppiel added this pull request to the merge queue May 10, 2026

Merged via the queue into main with commit eb9ad4f May 10, 2026
9 checks passed

danielmeppiel deleted the fix/794-global-lockfile-path branch May 10, 2026 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pass scope-resolved lockfile path to MCPIntegrator.update_lockfile at --global scope#1236

fix: pass scope-resolved lockfile path to MCPIntegrator.update_lockfile at --global scope#1236
danielmeppiel merged 2 commits intomainfrom
fix/794-global-lockfile-path

sergio-sisternes-epam commented May 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 10, 2026

Python Architect

CLI Logging Expert

DevX UX Expert

Supply Chain Security Expert

OSS Growth Hacker

Doc Writer

Test Coverage Expert

Auth Expert -- inactive

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

sergio-sisternes-epam commented May 10, 2026

Description

Type of change

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 10, 2026

APM Review Panel: ship_with_followups

Panel summary

Top 5 follow-ups

Architecture

Recommendation

Python Architect

CLI Logging Expert

DevX UX Expert

Supply Chain Security Expert

OSS Growth Hacker

Doc Writer

Test Coverage Expert

Auth Expert -- inactive

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

APM Review Panel: `ship_with_followups`