fix: address post-merge review on #542#547
Conversation
- Partition uninstall managed files with both default and resolved targets so user-scope paths are bucketed correctly - Pass resolved targets to orphan validate_deploy_path so user-scope paths are accepted for cleanup - Fix unused variable (error_msg -> _) - Consolidate CHANGELOG Fixed entries per repo convention Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Follow-up to #542 to address post-merge review items around user-scope uninstall/install cleanup, minor code cleanup, and changelog formatting consistency.
Changes:
- Update uninstall partitioning to bucket managed files against both default
KNOWN_TARGETSand scope-resolved targets, then merge the results. - Update orphan cleanup safety-gating to call
validate_deploy_path(..., targets=_targets)so user-scope prefixes are accepted. - Replace unused
error_msgvariables inskill_integratorwith_and consolidateCHANGELOG.mdfixed entries.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/apm_cli/integration/skill_integrator.py |
Removes unused error_msg binding from validate_skill_name call sites. |
src/apm_cli/commands/uninstall/engine.py |
Merges partition buckets from default targets and resolved targets to improve uninstall cleanup across legacy/user-scope paths. |
src/apm_cli/commands/install.py |
Passes resolved targets into validate_deploy_path during orphan cleanup to accept user-scope prefixes. |
CHANGELOG.md |
Consolidates multiple #542 Fixed bullets into a single entry per changelog conventions. |
| # Pass resolved targets so user-scope prefixes are accepted. | ||
| if BaseIntegrator.validate_deploy_path(_orphan_path, project_root, targets=_targets): |
There was a problem hiding this comment.
In user-scope installs, using only scope-resolved targets here means legacy orphaned paths that were written under project-scope prefixes (e.g., ".github/..." from older buggy user-scope installs) will fail validate_deploy_path() and will not be removed during orphan cleanup. Consider validating against both resolved targets and default KNOWN_TARGETS (or a union of allowed prefixes) so both ".copilot/..." and legacy ".github/..." orphan entries can be cleaned safely.
See below for a potential fix:
# Build validation targets that cover both current scope-resolved
# prefixes and default integrator targets so legacy project-scope
# orphaned paths (for example, ".github/...") can be removed safely.
_validation_targets = _targets or {}
try:
_default_targets = getattr(BaseIntegrator, "KNOWN_TARGETS", None)
except Exception:
_default_targets = None
if _default_targets:
_validation_targets = {**_default_targets, **_validation_targets}
for _orphan_path in sorted(orphaned_deployed_files):
# validate_deploy_path() is the safety gate: it rejects path-traversal,
# requires a known integration prefix, and checks the resolved path
# stays within project_root -- so rmtree is safe here.
# Validate against both default and scope-resolved targets so
# legacy project-scope prefixes are also cleaned up.
if BaseIntegrator.validate_deploy_path(_orphan_path, project_root, targets=_validation_targets):
| _buckets = BaseIntegrator.partition_managed_files(sync_managed) | ||
| if user_scope and _resolved_targets: | ||
| _scope_buckets = BaseIntegrator.partition_managed_files( | ||
| sync_managed, targets=_resolved_targets | ||
| ) |
There was a problem hiding this comment.
When user_scope is true, _resolved_targets is scope-resolved via TargetProfile.for_scope(), which filters out unsupported_user_primitives (e.g., Copilot removes "instructions"). As a result, partition_managed_files(..., targets=_resolved_targets) will not recognize/bucket managed paths for those filtered primitives under user-scope roots (e.g., legacy ".copilot/instructions/..." entries), so Phase 1 sync will never remove them even if they are present in all_deployed_files. If the intent is to clean up legacy installs, consider partitioning/cleanup against a target set that resolves user_root_dir but does not drop primitives (or otherwise explicitly include unsupported-primitive prefixes for removal only).
Merge KNOWN_TARGETS with scope-resolved targets for orphan cleanup validation, matching the pattern used in uninstall engine. This ensures legacy project-scope orphan paths (e.g., .github/...) are also cleaned up during user-scope installs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Follow-up to #542 addressing 4 post-merge review comments:
.copilot/agents/...are bucketed alongside legacy.github/agents/...pathsvalidate_deploy_pathso user-scope prefixes are acceptederror_msg->_in skill_integratorTesting