fix(lockfile): pass scope-resolved path to MCPIntegrator.update_lockfile#1230
Closed
Aaryan-Dadu wants to merge 2 commits intomicrosoft:mainfrom
Closed
fix(lockfile): pass scope-resolved path to MCPIntegrator.update_lockfile#1230Aaryan-Dadu wants to merge 2 commits intomicrosoft:mainfrom
Aaryan-Dadu wants to merge 2 commits intomicrosoft:mainfrom
Conversation
Aaryan-Dadu
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
Author
|
@microsoft-github-policy-service agree |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where MCP audit entries could be written to the wrong lockfile during apm install --global because MCPIntegrator.update_lockfile() defaulted to Path.cwd() when callers omitted the lock_path argument. The fix threads the already scope-resolved lockfile path into all relevant call sites so global installs update ~/.apm/apm.lock.yaml rather than a project-local lockfile.
Changes:
- Pass the scope-resolved lockfile path into
MCPIntegrator.update_lockfile()from the main install pipeline. - Pass the scope-resolved lockfile path into
MCPIntegrator.update_lockfile()from theapm install --mcpcommand path. - Add regression tests ensuring the scoped lockfile path is forwarded correctly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit/test_global_mcp_scope.py | Adds regression coverage asserting update_lockfile() is called with the scope-resolved lockfile path. |
| src/apm_cli/install/mcp/command.py | Ensures the --mcp install flow updates the correct lockfile by passing the computed lockfile path explicitly. |
| src/apm_cli/commands/install.py | Ensures the main install pipeline passes the scope-resolved lockfile path to all update_lockfile() call sites. |
MCPIntegrator.update_lockfile() defaults to get_lockfile_path(Path.cwd()) when no explicit path is passed. At --global scope, this cause MCP server audit entries to be written to the project-local apm.lock.yaml instead of the user-scope lockfile at ~/.apm/apm.lock.yaml. The caller already computes the scope-correct apm_dir (Path.home() / ".apm" for USER scope, Path.cwd() for PROJECT scope) and derives _lock_path from it. This change threads that path through all four update_lockfile call sites so the write always lands in the right file. Fixes: microsoft#794
This was referenced May 10, 2026
Three textual conflicts on the same microsoft#794 invariant: - src/apm_cli/commands/install.py: kept the explanatory comment describing why the scope-resolved _lock_path is passed explicitly. - src/apm_cli/install/mcp/command.py: aligned local var name to main's _mcp_lock_path while keeping the microsoft#794 explanatory comment. - tests/unit/test_global_mcp_scope.py: kept BOTH test classes -- TestUpdateLockfileScope (behavioral, mocks pipeline + scopes) and TestUpdateLockfileGlobalScope (AST contract on call sites). They pin the same invariant from different angles (defense in depth). Lint silent. 709 unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
|
Thanks for the fix @Aaryan-Dadu! Closing as superseded -- the same fix landed in #1236 (merged earlier today, closes #794). The diagnosis and call-site routing here matched #1236's approach almost exactly, so the underlying behavior is now correct on main. Really appreciate the contribution -- hope to see you back on another issue! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
MCPIntegrator.update_lockfilewas usingPath.cwd()as the default lockfile location even at--globalscope, causing MCP server audit entries to be written to the project-localapm.lock.yamlinstead of the user-scope~/.apm/apm.lock.yaml.Most caller in the install pipeline omitted the
lock_pathargument. SincePath.cwd()is always the current project directory (not the user's home), MCP entries for a global install were silently written to the wrong file.Fix
Thread the already-computed scope-resolved
_lock_path(derived fromctx.apm_dir, which is~/.apm/for--globalandPath.cwd()for project scope) into all fourupdate_lockfilecall sites.Fixes #794
Type of change
Testing