Skip to content

fix(uninstall): preserve unmanaged skills with an empty inventory - #2947

Open
Marco Frömbgen (mfroembgen) wants to merge 2 commits into
microsoft:mainfrom
mfroembgen:fix/preserve-unmanaged-skills
Open

fix(uninstall): preserve unmanaged skills with an empty inventory#2947
Marco Frömbgen (mfroembgen) wants to merge 2 commits into
microsoft:mainfrom
mfroembgen:fix/preserve-unmanaged-skills

Conversation

@mfroembgen

@mfroembgen Marco Frömbgen (mfroembgen) commented Sep 11, 2026

Copy link
Copy Markdown

Description

Uninstalling an MCP-only package could delete personal Claude and Kiro skills because an empty deployed_files inventory was converted to None, enabling legacy orphan cleanup. Preserve the empty inventory when a lockfile exists so cleanup only removes recorded resources. The existing fallback for installations without a lockfile remains unchanged.

Fixes #2946.

The lifecycle regression installs and uninstalls a local package with isolated agent settings. It verifies byte-for-byte preservation of personal .claude, .kiro, and .agents skills, removal of the package and its native MCP configuration, and normal managed-skill removal in a second case. The MCP-only case reproduced the deletion on upstream e38261c5 before the fix.

Type of change

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

Testing

  • Tested locally
  • Existing unit suite and relevant lifecycle tests pass
  • Added tests for new functionality (if applicable)

Validation on macOS with Python 3.12:

  • MCP-only lockfile lifecycle file: 10 passed, including both new regression cases, using the installed current-source CLI with an isolated HOME and disabled network.
  • Test quality suite: 63 passed; assertion and exact-duplicate ratchets passed.
  • Full Ruff check/format, pylint duplication, architecture, auth, YAML I/O, file-length, and portable-path guards passed.
  • Full unit suite (uv run --no-sync pytest tests/unit tests/test_console.py -x -q): 22,299 passed, 42 skipped, 21 expected failures, and 99 subtests passed. The initial parallel run hit a temporary-directory assertion in test_detect_cleans_temp_dir_on_malicious_archive; the complete serial run passed.

To run the lifecycle coverage:

APM_E2E_TESTS=1 uv run --extra dev pytest -q tests/integration/test_mcp_only_lockfile_lifecycle.py

Spec conformance (OpenAPM v0.1)

This restores the existing documented uninstall ownership contract; it introduces no new requirement or schema.

apm-spec-waiver: Restore documented lockfile-scoped uninstall cleanup for an empty deployment inventory; no new OpenAPM contract or schema.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved review issues remain, and all approval-readiness assessments indicate approval.

Pull request overview

Fixes uninstall cleanup so MCP-only packages preserve unmanaged skills when the lockfile has an empty deployment inventory.

Changes:

  • Preserve empty lockfile-managed inventories.
  • Add lifecycle regression coverage.
  • Document the fix.
File summaries
File Description
tests/integration/test_mcp_only_lockfile_lifecycle.py Verifies unmanaged skills survive and managed skills are removed.
src/apm_cli/commands/uninstall/engine.py Preserves lockfile-scoped cleanup behavior.
CHANGELOG.md Documents the bug fix.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@mfroembgen

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] MCP-only package uninstall deletes unmanaged Claude and Kiro skills

2 participants