fix: improve sub-skill overwrite UX with content skip and collision protection

[danielmeppiel]

Problem

When running apm install, sub-skill overwrite warnings were:

1. Not actionable — message said "overwrites existing skill" but the overwrite always proceeded regardless. Nothing for the user to do.
2. Cluttering inline output — yellow warnings interleaved between green checkmarks and summary lines, breaking the visual flow.
3. Missing collision protection — unlike all other integrators, skills had no managed_files/force check, silently destroying user-authored skills at the same path.

Solution

Three fixes applied to _promote_sub_skills and its call chain:

1. Content-identical skip

Before any collision logic, compare source and target directories using filecmp.cmpfiles(shallow=False). If identical, skip entirely — no copy, no warning, no noise. Handles the common re-install case silently.

2. User-authored collision protection

Thread managed_files and force through the skill integrator call chain (matching the pattern of all other integrators). If target exists and is NOT in managed_files, skip with actionable warning: use --force to overwrite.

3. Diagnostics wiring + wording

Pass diagnostics to all 3 integrate_package_skill() call sites so cross-package overwrites appear in the end-of-install grouped summary instead of inline. Updated wording from "sub-skills overwrote existing skills" to "skills replaced by a different package (last installed wins)".

Before / After

Before (re-install, nothing changed):

checkmark github.com/github/awesome-copilot/plugins/testing-automation
Sub-skill 'playwright-explore-website' from 'testing-automation' overwrites existing skill...
Sub-skill 'csharp-nunit' from 'testing-automation' overwrites existing skill...
Sub-skill 'java-junit' from 'testing-automation' overwrites existing skill...

After (re-install, nothing changed): Content-identical skip — zero noise.

checkmark github.com/github/awesome-copilot/plugins/testing-automation
  sub-item 3 skill(s) integrated -> .github/skills/

Changes

File	Change
skill_integrator.py	_dirs_equal helper, content skip, managed_files/force collision protection, threaded params through call chain
install.py	Pass diagnostics, managed_files, force to all 3 integrate_package_skill() call sites
diagnostics.py	Updated overwrite group wording
cli-commands.md	Synced diagnostic summary description
test_skill_integrator.py	6 new tests: content skip, collision protection, force override, diagnostics routing, self-overwrite silence
test_diagnostics.py	Updated assertion for new wording

[Copilot]

Pull request overview

This PR improves the apm install experience for promoted sub-skills by reducing noisy overwrite warnings, adding content-based no-op behavior, and introducing collision protection plumbing consistent with the rest of the install pipeline.

Changes:

• Added directory-content equality checks to skip re-promoting sub-skills when source/target contents are identical.
• Threaded diagnostics, managed_files, and force through skill integration call paths to support deferred reporting and collision handling.
• Updated diagnostic wording + docs, and added/updated unit tests for the new behavior.

Reviewed changes

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

Show a summary per file

File	Description
src/apm_cli/integration/skill_integrator.py	Adds directory comparison + collision/diagnostics logic to sub-skill promotion and threads new params through the integrator call chain.
src/apm_cli/commands/install.py	Passes diagnostics, managed_files, and force into skill integration calls.
src/apm_cli/utils/diagnostics.py	Updates overwrite summary wording and changes verbose rendering behavior.
docs/src/content/docs/reference/cli-commands.md	Updates docs to reflect the new diagnostic category wording.
tests/unit/integration/test_skill_integrator.py	Adds tests covering content-identical skip, collision behavior, and diagnostics routing.
tests/unit/test_diagnostics.py	Updates assertions for the new overwrite wording.

Comments suppressed due to low confidence (3)

src/apm_cli/integration/skill_integrator.py:521

• _promote_sub_skills() reimplements user-authored collision detection instead of using BaseIntegrator.check_collision() even though SkillIntegrator extends BaseIntegrator. Duplicating this logic increases drift risk and makes it harder to keep collision behavior consistent across integrators (e.g., path normalization, diagnostics formatting). Suggest delegating to self.check_collision(...) (or BaseIntegrator.check_collision) and centralizing any skill-specific behavior around it.

                # Check if this is a user-authored skill (not managed by APM)
                rel_dir = f".github/skills/{sub_name}"
                is_managed = (
                    managed_files is not None
                    and rel_dir.replace("\\", "/") in managed_files
                )
                prev_owner = (owned_by or {}).get(sub_name)
                is_self_overwrite = prev_owner is not None and prev_owner == parent_name

                if managed_files is not None and not is_managed and not is_self_overwrite:
                    # User-authored skill — respect force flag
                    if not force:
                        if diagnostics is not None:
                            diagnostics.skip(
                                f"skills/{sub_name}/", package=parent_name
                            )
                        else:
                            try:
                                from apm_cli.utils.console import _rich_warning
                                _rich_warning(
                                    f"Skipping skill '{sub_name}' — local skill exists (not managed by APM). "
                                    f"Use 'apm install --force' to overwrite."
                                )
                            except ImportError:
                                pass
                        continue  # SKIP — protect user content

src/apm_cli/integration/skill_integrator.py:493

• _dirs_equal() assumes both inputs are directories. In _promote_sub_skills() this is called when target.exists(), but target could be a file or symlink (user-created), which would raise from filecmp.dircmp before the collision-protection logic runs. Suggest guarding with target.is_dir() (treat non-dir as not equal) and handling non-directory targets explicitly (skip unless --force, or remove with unlink when forcing).

            if target.exists():
                # Content-identical → skip entirely (no copy, no warning)
                if SkillIntegrator._dirs_equal(sub_skill_path, target):
                    promoted += 1
                    deployed.append(target)

src/apm_cli/integration/skill_integrator.py:703

• In _integrate_native_skill(), managed_files/force are now threaded through, but within this method they’re only used for _promote_sub_skills(...) (sub-skill promotion). The main skill directory copy logic above still overwrites any existing .github/skills/<skill_name> / .claude/skills/<skill_name> unconditionally, so user-authored root-skill collisions remain unprotected. Consider applying the same collision semantics to the root skill directory too (ideally via BaseIntegrator.check_collision).

        sub_skills_dir = package_path / ".apm" / "skills"
        github_skills_root = project_root / ".github" / "skills"
        owned_by = self._build_skill_ownership_map(project_root)
        sub_skills_count, sub_deployed = self._promote_sub_skills(sub_skills_dir, github_skills_root, skill_name, warn=True, owned_by=owned_by, diagnostics=diagnostics, managed_files=managed_files, force=force)