fix(prune): detect orphan skills and preserve retained bundles - #3057
fangkang (fangkangmi) wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Orphan detection still risks masking real SKILL.md-only install roots due to standalone classification not considering SKILL.md, and the new lifecycle integration module is missing the established e2e marker.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR fixes apm prune orphan detection for manifestless skill installs by treating SKILL.md as an install marker, while ensuring skills nested under declared/retained roots (e.g., manifestless bundles) are preserved. It also centralizes orphan selection logic so both prune and advisory warnings route through the same declaration-aware selector, with architecture-owner + guard coverage.
Changes:
- Extend installed-package scanning to recognize
SKILL.mdand treat nested skills as owned by their parent package boundary. - Share orphan selection via
_find_orphaned_packages()and enforce it via a new architecture linter rule/owner record. - Add unit + integration lifecycle tests for orphan skill cleanup and retained bundle/sibling preservation; update prune docs and changelog.
| File | Description |
|---|---|
src/apm_cli/commands/deps/_utils.py |
Teach _scan_installed_packages() / nesting detection to treat SKILL.md as a package marker. |
src/apm_cli/commands/_helpers.py |
Centralize orphan selection in _find_orphaned_packages() and reuse it from _check_orphaned_packages(). |
src/apm_cli/commands/prune.py |
Use _find_orphaned_packages() to match advisory/orphan warning selection semantics. |
tests/unit/test_deps_utils.py |
Add unit coverage for skill-only packages and embedded-skill ownership under a parent marker. |
tests/unit/test_prune_command.py |
Add unit coverage for pruning skill-only installs, preserving siblings, embedded skills, and manifestless bundles. |
tests/integration/test_prune_skill_lifecycle.py |
Add real CLI lifecycle integration coverage for install/remove/prune and declared bundle preservation. |
scripts/architecture_linter/checks/install_uninstall_and_resolution.py |
Add install-deployment-orphan-selection guard enforcing routing through _find_orphaned_packages(). |
scripts/architecture_linter/checks/install_deployment_analyzers.py |
Register the new orphan-selection architecture rule. |
tests/unit/scripts/test_architecture_runner.py |
Include the new orphan-selection guard in the architecture runner expectations. |
tests/integration/test_architecture_owner_rule_mutations.py |
Add mutation case ensuring prune can’t bypass the shared orphan selector. |
.apm/architecture/owners/install-deployment.json |
Register the orphan selection decision owner and guard. |
docs/src/content/docs/reference/cli/prune.md |
Document manifestless SKILL.md orphan detection + retained bundle preservation behavior. |
CHANGELOG.md |
Add a Fixed entry for the prune behavior change (#3015). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| orphaned_packages = _find_orphaned_packages( | ||
| installed_packages, expected_installed, standalone_installed | ||
| ) |
There was a problem hiding this comment.
Fixed in 00016f6. Standalone detection now recognizes SKILL.md; added regression coverage for prune and dry-run.
|
Thank you for this pull request. It is linked to #3015, which a maintainer has accepted for implementation. This is advisory triage only, not merge or scope approval. CODEOWNERS review remains with Daniel Meppiel (@danielmeppiel) and Sergio Sisternes (@sergio-sisternes-epam) (already requested). A maintainer still has to review the diff. Suggested next step: wait for CODEOWNERS review. Copilot left inline notes; the author replied that SKILL.md standalone detection and the e2e marker were addressed in 00016f6. A reviewer should confirm those threads. Generated by autopilot-pr-triage-worker. This comment is AI-generated and may contain errors. |
Sergio Sisternes (sergio-sisternes-epam)
added
triage/recommended
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| python-architect | 0 | 1 | 1 | Canonical orphan selection correctly centralized; ancestor-of-declared-child retention gap remains (known_gaps[0]). |
| test-coverage-expert | 1 | 1 | 0 | Blocking: test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory codifies safe_rmtree on ancestor containing a still-declared child; _find_orphaned_packages lacks reverse ancestor check. Windows lifecycle file not collected by Windows CI job. |
| devx-ux-expert | 1 | 0 | 1 | Prune destroys declared child content when an ancestor skill root is marked orphan; blocks the package-manager cleanup contract. |
| supply-chain-security-expert | 1 | 0 | 1 | Newly reachable SKILL.md orphan-root deletion path can destroy declared child content; reverse-ancestor guard required before merge. |
| performance-expert | 0 | 2 | 0 | New retained-ancestor prefix loop is O(IE) where O(ID) suffices; prune path doubles _expand_with_ancestors call. Both are sub-ms today but compound with dep count. |
| doc-writer | 0 | 3 | 0 | Clarify managed-root deletion, needed-ancestor retention, and Windows test selection. Static review only; no tests executed. |
| cli-logging-expert | 0 | 0 | 0 | Shared _find_orphaned_packages gives compile-warning/prune parity; found/removed/failed totals are truthful; dry-run is nonmutating; no false-clean claims. Ship. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [test-coverage-expert + devx-ux-expert + supply-chain-security-expert + python-architect] (blocking-severity) Implement reverse-ancestor guard in
_find_orphaned_packagesand reversetest_skill_only_orphan_root_is_not_hidden_by_declared_subdirectoryto assert ancestor retention -- Net-new regression: SKILL.md recognition makes the ancestor-deletion path reachable for the first time. Four panelists converge onnot any(root.startswith(f"{path}/") for root in expected)with indexed segment-bounded prefix walk (frozenset, O(ID)) replacing the O(IE) linear scan. Test must assertroot.exists()unconditionally and verify child content survives prune. - [test-coverage-expert] (blocking-severity) Expand Windows CI collection roots to
tests/unit tests/integrationunder existingwindows_compatmarker before convergence -- Issue [BUG] apm prune does not delete packages from apm_modules #3015 was reported on Windows 11 / Python 3.13. The lifecycle file carrieswindows_compatbut ci.yml:156 only collectstests/unit tests/integration/test_lifecycle_workspace_lock.py. Mandatory before this PR converges -- same job, permissions, and timeout; marker gates execution scope. - [test-coverage-expert + devx-ux-expert] (blocking-severity) Add CLI lifecycle smoke tests: install -> remove -> prune sequence, dry-run nonmutation, retained roots/children safety, and exact-head owner-v2 semantic verification -- Brief known_gaps require actual CLI lifecycle coverage beyond the unit-level orphan selector. Reversible functional mutations (dry-run produces no FS delta), repeated prune idempotency, and the canonical owner guard's version-2 behavioral + static contract need integration-tier evidence on the commit that ships.
- [doc-writer] Update prune.md: state managed-root deletion tradeoff, replace ancestor-deletion note with retention semantics alongside code fix, consolidate duplicate prune row in commands.md -- Current note at prune.md:128 implies SKILL.md detection is protective but feeds the deletion path for ancestor roots. Doc must not claim ancestor retention before the code fix lands. Consolidate commands.md:25-27 duplicate prune rows to offset added prose. Update CHANGELOG:44 to cover both manifestless cleanup and ancestor preservation.
- [python-architect] Extend architecture mutation test to cover
_check_orphaned_packagescall site in addition to existingprune.pysite -- The dual-site orphan-selection guard (check_orphan_selection) monitors both paths but the mutation test only mutates prune.py. Adding a matchingMutationCasefor_check_orphaned_packagescloses verification for both monitored call sites.
Architecture
classDiagram
direction LR
class _helpers_py {
<<Module>>
+_check_orphaned_packages() list
+_find_orphaned_packages(installed, expected, standalone) list
+_expand_with_ancestors(paths, installed) set
+_standalone_installed_packages(installed, dir, lockfile) list
}
class prune_py {
<<Module>>
+prune(ctx, dry_run)
}
class deps_utils_py {
<<Module>>
+_scan_installed_packages(apm_modules_dir) list
+_is_nested_under_package(candidate, root) bool
}
class install_uninstall_resolution_py {
<<StaticGuard>>
+check_orphan_selection(provider) tuple
}
class install_deployment_json {
<<OwnerRegistry>>
orphan-package-selection
}
note for _helpers_py "Pure Function: _find_orphaned_packages\ncanonical orphan selector"
note for install_uninstall_resolution_py "Boundary Check:\nboth call sites must route\nthrough _find_orphaned_packages"
prune_py ..> _helpers_py : imports _find_orphaned_packages
_helpers_py ..> deps_utils_py : imports _scan_installed_packages
install_uninstall_resolution_py ..> _helpers_py : verifies calls
install_uninstall_resolution_py ..> prune_py : verifies calls
install_deployment_json ..> install_uninstall_resolution_py : guard reference
class _helpers_py:::touched
class prune_py:::touched
class deps_utils_py:::touched
class install_uninstall_resolution_py:::touched
class install_deployment_json:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["CLI: apm prune"] --> B["prune.py:prune()"]
W["_helpers.py:_check_orphaned_packages()"] --> K
B --> C["[FS] deps/_utils.py:_scan_installed_packages\nRecognizes apm.yml / .apm / SKILL.md"]
B --> D["[I/O] _helpers.py:_standalone_installed_packages\napm.yml OR SKILL.md.is_file() at line 288-292"]
C --> E["installed list"]
D --> F["standalone list"]
E --> K["_helpers.py:_find_orphaned_packages\nline 340"]
F --> K
K --> L["_expand_with_ancestors line 171\nexpected + namespace ancestors"]
L --> M{"ancestor in standalone\nAND not declared?\nline 244"}
M -->|Skip| N["Ancestor excluded\nowner/repo with SKILL.md skipped"]
M -->|Add| O["Ancestor added to expansion"]
K --> P{"path NOT in expected_with_ancestors?\nline 353"}
P -->|In set| Q["Retained"]
P -->|Not in set| R{"path is child of expected root?\nstartswith check line 355"}
R -->|Child| S["Retained: child protected"]
R -->|Not child| T["GAP: ancestor of expected\nnot protected"]
T --> U["Orphan selected"]
U --> V{"dry_run?"}
V -->|No| X["[FS] safe_rmtree orphan_dir\nDestroys declared children inside"]
V -->|Yes| Y["Report only"]
Recommendation
The child-deletion gap is a concrete net-new regression -- safe_rmtree on a SKILL.md ancestor destroys still-declared children on a path that was unreachable before this PR's scanner enhancement. Driver should merge main faithfully, implement the reverse-ancestor guard with the indexed prefix walk, reverse the test assertion, expand Windows CI collection roots, and add lifecycle smoke tests covering the install -> remove -> prune sequence with retained-root/child safety and owner-v2 semantic verification. fangkangmi's architectural contribution (centralized _find_orphaned_packages, SKILL.md recognition) is sound and worth shipping; the fix is a predicate addition plus a test reversal, not a redesign.
Full per-persona findings
python-architect
- [recommended] Standalone SKILL.md ancestor containing a declared child is orphaned and deleted, destroying the child. at
src/apm_cli/commands/_helpers.py:355
_find_orphaned_packages (line 355) protects children of expected roots viapath.startswith(f"{root}/")but does not protect ancestors that CONTAIN expected children. _expand_with_ancestors (line 244) skips standalone ancestors (ancestor in installed_set and ancestor not in materialized_set). In the scenario whereowner/repohas SKILL.md (newly recognized) andowner/repo/skills/childis declared,owner/repois standalone, excluded from ancestor expansion, classified as orphan, and deleted by safe_rmtree -- destroying the declared child. Before this PR, SKILL.md was not recognized by _scan_installed_packages, so the root was invisible to prune and the child was safe. The test test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory (line 391) encodes this broken behavior. Brief known_gaps[0] calls this 'Required change is ancestor retention, no waiver as inherited behavior.' The approved plan addresses it with 'indexed retained ancestors'. A reverse-containment guard in _find_orphaned_packages (not any(root.startswith(f"{path}/") for root in expected)) would close the gap.
Design patterns
- Used in this PR: Pure Function extraction --
_find_orphaned_packagescentralizes orphan selection as a side-effect-free function shared by_check_orphaned_packages(warning) andprune(deletion), eliminating the prior split authority. - Used in this PR: Static Boundary Guard --
check_orphan_selection+ mutation test enforces both call sites route through the canonical owner, registered ininstall-deployment.json. - Pragmatic suggestion: none -- the current shape is the simplest correct design at this scope; the ancestor fix is a predicate addition, not a pattern change.
Suggested: Add reverse-containment check:and not any(root.startswith(f"{path}/") for root in expected). This protects ancestors that contain declared children without weakening sibling orphan detection. Update test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory to assert ancestor retention instead of deletion. - [nit] Architecture mutation test covers only the prune.py call site of the dual-site orphan-selection guard. at
tests/integration/test_architecture_owner_rule_mutations.py:78
check_orphan_selection verifies both _check_orphaned_packages and prune route through _find_orphaned_packages. The mutation test (test_architecture_owner_rule_mutations.py) only mutates prune.py. Adding a matching MutationCase for _check_orphaned_packages would close the guard verification for both monitored paths.
test-coverage-expert
- [blocking] test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory asserts recursive deletion of an ancestor whose subtree contains a still-declared child dependency at
tests/unit/test_prune_command.py:391
Traced _find_orphaned_packages for the test scenario: installed=['owner/repo','owner/repo/skills/child'], expected={'owner/repo/skills/child'}, standalone=['owner/repo']. _expand_with_ancestors SKIPS 'owner/repo' as a standalone ancestor (not declared), producing expected_with_ancestors={'owner/repo/skills','owner/repo/skills/child'}. The child-protection filter checks path.startswith(f'{root}/') for root in expected, but 'owner/repo'.startswith('owner/repo/skills/child/') is False, so 'owner/repo' passes through as an orphan. safe_rmtree (wrapping shutil.rmtree) on apm_modules/owner/repo recursively deletes the entire tree including owner/repo/skills/child -- a DECLARED dependency. The test codifies this with assert root.exists() == dry_run and never checks child.exists() in non-dry-run mode. The code is missing the symmetric reverse check: not any(root.startswith(f'{path}/') for root in expected), which would protect ancestors containing declared descendants. The brief explicitly requires: 'current unsafe expected child deletion must be reversed' and 'Keep such ancestor roots intact; no selective descendant carve-out.' No test at any tier asserts the correct behavior (ancestor retention when containing a declared child). grep'd tests/ for ancestor.*retain, retain.*ancestor, protect.*ancestor: no match asserting retention.
Suggested: Fix _find_orphaned_packages in src/apm_cli/commands/_helpers.py to add the reverse ancestor check: 'and not any(root.startswith(f"{path}/") for root in expected)'. Then reverse the test assertion to: assert root.exists() (root is retained in all modes), assert (child / "SKILL.md").read_text() == "# Child skill\n", assert "No orphaned packages" in result.output. Add a corresponding lifecycle-tier test in tests/integration/test_prune_skill_lifecycle.py (e.g. test_prune_retains_standalone_ancestor_containing_declared_child) using real git install + declaration removal + prune to prove the ancestor survives with its declared child intact.
Evidence (missing, integration-with-fixtures):tests/unit/test_prune_command.py-- Prune never removes a standalone ancestor directory when it contains a still-declared child dependency - [recommended] Windows CI collection roots do not include tests/integration/test_prune_skill_lifecycle.py despite windows_compat marker on the new lifecycle file at
.github/workflows/ci.yml:156
The windows-compat-gate CI job collects 'tests/unit tests/integration/test_lifecycle_workspace_lock.py' at line 154-156 of ci.yml. The new test_prune_skill_lifecycle.py carries pytestmark=[pytest.mark.windows_compat] at module scope, but the marker only filters WITHIN collected paths -- it does not expand the collection. The lifecycle tests will not execute on Windows CI. The original issue [BUG] apm prune does not delete packages from apm_modules #3015 was reported on Windows 11/Python 3.13. The approved plan acknowledges this: 'Bounded Windows repair changes collection roots to tests/unit tests/integration under existing windows_compat marker, same job/permissions/timeout.' Until that repair lands, the SKILL.md recognition and prune lifecycle contracts have no Windows execution evidence. Existing test_prune_failure_lifecycle.py and test_prune_hook_reconciliation_e2e.py in tests/integration are also outside the collection roots and likewise do not run on Windows.
Suggested: Expand collection roots in windows-compat-gate to 'tests/unit tests/integration' (the approved plan). Verify the windows_compat marker gates appropriately so only marked tests execute, keeping job runtime bounded. This is planned work; do not block on it, but track it as a required follow-up before the next release.
Evidence (unknown, integration-with-fixtures):tests/integration/test_prune_skill_lifecycle.py-- Prune skill lifecycle contracts execute on Windows where the original issue 3015 was reported
devx-ux-expert
- [blocking] _find_orphaned_packages marks ancestor roots as orphans even when they contain a still-declared child, causing safe_rmtree to destroy declared dependency content at
src/apm_cli/commands/_helpers.py:355
Package manager cleanup invariant: prune removes only content the user no longer needs. npm prune, pip, cargo clean -- none delete content still declared in the manifest. _find_orphaned_packages (line 355) guards descendants of expected roots (path.startswith(root/)) but never checks the reverse: whether any expected path is a descendant of the candidate orphan. When owner/repo has SKILL.md (standalone) and owner/repo/skills/child is declared, owner/repo is correctly excluded from ancestor expansion (installed but not declared), but then nothing prevents it from being marked orphaned. safe_rmtree on apm_modules/owner/repo recursively deletes the entire tree including the declared child. The user's next apm compile or apm run fails with missing dependency content, and recovery requires a full apm install. The test test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory (line 408: assert root.exists() == dry_run) encodes this destructive behavior as correct, contradicting the brief's required ancestor retention policy.
Suggested: Add a reverse-descendant guard:and not any(root.startswith(f"{path}/") for root in expected). Fix the test at test_prune_command.py:408 to assert ancestor retention when it contains a declared child.
Evidence (code-read only; no execution, unit):tests/unit/test_prune_command.py-- Test encodes destructive behavior: prune destroys an ancestor root whose subtree contains a declared dependency, violating the cleanup-only contract. - [nit] prune.md note implies SKILL.md detection is protective but the detection feeds the deletion path for ancestor roots at
docs/src/content/docs/reference/cli/prune.md:128
The new note at line 128 ('Manifestless installs containing SKILL.md are detected even after apm install removes their lockfile entry') reads as a safety assurance: we find these packages so they participate in cleanup decisions. But detection of a SKILL.md root is what makes it standalone, which is what makes it eligible for orphan marking, which triggers deletion even when it contains declared children (see blocking finding). Until the ancestor guard is fixed, 'detected' effectively means 'eligible for deletion' for ancestor roots. The note is not technically false but primes the user to expect safe handling that does not exist.
supply-chain-security-expert
- [blocking] Pruning a SKILL.md-recognized standalone orphan root recursively destroys its declared child because _find_orphaned_packages lacks a reverse-ancestor containment guard at
src/apm_cli/commands/_helpers.py:355
_find_orphaned_packages (line 340) protects descendants of expected roots viapath.startswith(f"{root}/"), but never checks whether a candidate orphan is an ANCESTOR of expected content. When a SKILL.md-bearing root (e.g. owner/repo) is classified standalone by the new scanner, _expand_with_ancestors correctly excludes it from ancestor expansion (it is standalone, not an intermediary). But _find_orphaned_packages then classifies the root as orphaned because it is not in expected_with_ancestors and is not a descendant of any expected path. prune.py line 254 calls safe_rmtree on the entire subtree, destroying the declared child (e.g. owner/repo/skills/child) along with the root. safe_rmtree's ensure_path_within guard only enforces containment within apm_modules -- it has no visibility into whether the subtree contains declared dependencies. This is a newly introduced reachable path: before this PR, directories with only SKILL.md were invisible to _scan_installed_packages, so this ancestor-deletion scenario was unreachable. The test test_skill_only_orphan_root_is_not_hidden_by_declared_subdirectory codifies the destructive behavior withassert root.exists() == dry_run. The brief's known_gaps[0] confirms: 'Required change is ancestor retention, no waiver as inherited behavior.'
Suggested: Add a reverse-ancestor guard to _find_orphaned_packages: before classifying a path as orphaned, checknot any(root.startswith(f"{path}/") for root in expected)to protect ancestors of declared content. Fix the test assertion to expect ancestor retention (root.exists() == True regardless of dry_run). This keeps the standalone-orphan detection for genuine orphans while preventing recursive destruction of declared descendants.
Evidence (unknown, unit):tests/unit/test_prune_command.py-- Test codifies destructive ancestor deletion of declared child content; if it passes, it proves the declared-content-preservation contract is violated. - [nit] Prior panel-finding audit trail about forgeable marker heuristic removed from _standalone_installed_packages docstring at
src/apm_cli/commands/_helpers.py:265
The old docstring explicitly acknowledged a prior panel finding: 'Combining both signals closes the suppression-via-absence gap (panel finding: forgeable apm.yml heuristic)'. This was replaced with 'Package markers preserve standalone orphan detection even when a declaration points at a subdirectory of a removed package root.' With SKILL.md now extending the forgeable marker surface (an attacker who can write into apm_modules/ can drop either apm.yml or SKILL.md to influence standalone classification), preserving the forgeable-marker acknowledgment keeps the known weakness visible to future security reviewers without changing any runtime behavior.
performance-expert
-
[recommended] O(IE) linear prefix scan in _find_orphaned_packages should be O(ID) via set-indexed segment walk at
src/apm_cli/commands/_helpers.py:355
Line 355 iterates every root inexpectedfor every installed path:any(path.startswith(f"{root}/") for root in expected). This is O(IEL) where I=installed, E=expected, L=avg string length. For 100 installed x 80 expected x 30-char avg that is ~240k char comparisons per invocation. The function runs on both the warning path (compile/install via _check_orphaned_packages) and the deletion path (prune), so the cost is paid on every CLI command that checks for orphans. The expected set is already typed as set[str]; splitting each installed path into segments and checking each segment-delimited prefix against the set replaces the inner O(E) scan with O(D) set lookups (D = path depth, bounded at 2-6 by install conventions). This is the canonical 'x in list inside a loop -> recommend set/dict index' pattern from the algorithmic-patterns lens. Absolute cost today is sub-ms for typical projects, but the pattern compounds linearly with dep count and is trivially fixable.
Suggested: Replace the innerany(path.startswith(...))with a segment-bounded prefix walk against a frozenset:expected_frozen = frozenset(expected)
def _is_child_of_expected(p: str) -> bool:
parts = p.split("/")
return any("/".join(parts[:i]) in expected_frozen for i in range(1, len(parts)))return sorted(
path for path in installed
if path not in expected_with_ancestors
and not _is_child_of_expected(path)
)
Build expected_frozen once outside the comprehension. Complexity drops from O(IE) to O(ID) where D<=6. Scaling guard: parameterize a benchmark at N and 10*N expected paths, assert wall-time ratio stays below 2x.
-
[recommended] Redundant _expand_with_ancestors call: prune.py:134 and _find_orphaned_packages:350 compute the identical expansion at
src/apm_cli/commands/prune.py:148
prune.py line 134 calls _expand_with_ancestors(expected_installed, standalone_installed) and stores the result in expected_with_ancestors for the missing_orphaned_keys comprehension at line 155. Then line 148 calls _find_orphaned_packages which internally calls _expand_with_ancestors(expected, standalone) with the same two arguments, computing the identical set a second time. _expand_with_ancestors does list+set construction, path validation via validate_path_segments (regex), string splitting, and up to 3 ancestor join operations per expected path -- roughly O(E*3) string ops + O(S) set construction per call. For 80 expected + 50 standalone this is ~500 operations duplicated. The fix is to accept an optional pre-computed expanded set in _find_orphaned_packages, or restructure prune.py to pass its already-computed result. The warning path (_check_orphaned_packages) does not have this duplication since it only calls _find_orphaned_packages once, so the fix is prune-specific. Redundant computation pattern from the algorithmic lens.
Suggested: Add an optional expected_with_ancestors parameter to _find_orphaned_packages:def _find_orphaned_packages(
installed, expected, standalone,
*, _precomputed_ancestors: set[str] | None = None
) -> list[str]:
expanded = _precomputed_ancestors or _expand_with_ancestors(expected, standalone)
...
In prune.py pass the already-computed set:
orphaned_packages = _find_orphaned_packages(
installed_packages, expected_installed, standalone_installed,
_precomputed_ancestors=expected_with_ancestors,
)
_check_orphaned_packages continues without the kwarg and pays the single call.
doc-writer
- [recommended] State the approved whole-root deletion tradeoff. at
docs/src/content/docs/reference/cli/prune.md:128
The new note explains SKILL.md discovery but omits its user-visible consequence. src/apm_cli/commands/deps/_utils.py:29-34 recognizes roots by package markers, and src/apm_cli/commands/prune.py:250-254 removes the selected directory wholesale. Manually copied packages and personal files inside an eligible root can therefore be removed without a surviving lock entry. The deployment-ownership protections described elsewhere on this page do not protect those source-root contents.
Suggested: Add a terse sentence near the orphan definition: "Recognized roots under apm_modules/ are managed installation content. Pruning an eligible root removes its contents, including manually added files; preview with apm prune --dry-run." Keep this definition canonical. Consolidate the duplicate prune rows in packages/apm-guide/.apm/skills/apm-usage/commands.md:25-27 into one short summary and reference, offsetting the added prose. Do not introduce receipt or .apm-pin ownership requirements. - [recommended] Replace the ancestor-deletion note alongside the retention fix. at
docs/src/content/docs/reference/cli/prune.md:131
The revised note still endorses removing a recognized ancestor of a needed subdirectory. That contradicts the approved requirement to retain the entire ancestor root. Current src/apm_cli/commands/_helpers.py:238-245 excludes recognized ancestors from protection, while :350-355 protects descendants of expected roots, not ancestors containing them. This is not resolved by documenting bundle-descendant preservation alone.
Suggested: When the driver fixes selection, replace this implementation-oriented bullet with: "Declared direct and dev dependencies, retained transitives, and their bundled skills are preserved. An ancestor root containing a needed dependency is retained intact; an unrelated sibling root remains eligible for pruning." Do not claim this behavior before the code fix. Update the existing CHANGELOG.md:44 entry to summarize both manifestless cleanup and needed-child/ancestor preservation rather than adding another entry. - [recommended] Distinguish Windows marker selection from Windows execution evidence. at
docs/src/content/docs/contributing/integration-testing.md:386
The contributor guidance mentions bounded collection roots without naming them. .github/workflows/ci.yml:152-157 currently filters windows_compat only within tests/unit and tests/integration/test_lifecycle_workspace_lock.py. Marking a different integration file does not include it in that job. The supplied packet identifies precisely this lifecycle-selection gap; the marker itself establishes neither collection nor a successful Windows run.
Suggested: Replace the paragraph alongside the approved CI repair with the exact selection: windows_compat within tests/unit and tests/integration. Explain briefly that collection confirms inclusion, while a successful Windows job provides execution evidence. Retain the non-empty-subset and timeout guidance, and do not describe the widened roots as current until ci.yml changes.
cli-logging-expert
No findings.
This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.
Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep direct, dev and transitive contents under recognized roots; use one indexed orphan selector for warnings, source deletion and stale lock cleanup. Exercise installed CLI lifecycles, retry failures and marker-scoped Windows coverage without requiring cache pins as ownership proof. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The existing fixture installs alpha-kit and beta-kit aliases but asserted nonexistent org/repo paths. Resolve paths from installed lock entries, assert both existed before prune, then assert actual removal and byte-for-byte survivor retention. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…p docs Preserve the existing deterministic mutation ordering. Align consumer and lockfile guidance with recognized-root retention and the unchanged trusted-deployment cleanup boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Fixes #3015:
apm prunenow detects orphanedSKILL.mdpackages after their lockfile entries are removed, while preserving skills inside declared or transitive bundles. Prune and advisory warnings share the same selection logic, including standalone skill-root detection.Sergio's original informal approval and the earlier bounded scope remain preserved in the issue history. They are historical context; the sole current scope nomination is below.
Approved managed-install-root policy and lifecycle acceptance criteria. The maintainer explicitly approved npm-style cleanup of recognized package roots inside
apm_modules/, including the risk to manually copied packages or personal files inside a removable root. No historical receipt or.apm-pinownership proof is required. Still-needed direct/transitive packages, bundled skills, nested children and their containing roots must be preserved; deployed files and shared configuration outsideapm_modules/keep their existing ownership safeguards.The SAME plan-first implementation driver is authorized to revise its plan, resolve current-main conflicts, fold in-scope fixes and provide final-head lifecycle evidence, including the original install/remove/install/prune sequence and actual affected Windows execution. The revised executable plan still needs app approval. Preserve fangkang (@fangkangmi)'s contribution; no new receipt system, cache-pin ownership reinterpretation, replacement PR, force-push, approval review or merge is authorized. The evidence below belongs to the contributor's current revision, not the driver's final validation.
Validation
e2emarker. Windows execution remains unverified.Remaining limits
The note below records the contributor's pre-policy limitations. The manual-content trade-off is now explicitly accepted within removable managed package roots only; deletion of a still-needed child or its containing root is NOT accepted and remains to be corrected and verified.
Ownership remains unresolved: manually copied skills inside
apm_modules/can be deleted without proof that APM installed them. Standalone-root pruning also follows existing whole-directory semantics, including children sharing that root.