From 418c4f0d33ab73ba1b66f56a2633634efbbd586f Mon Sep 17 00:00:00 2001
From: danielmeppiel <dmeppiel@microsoft.com>
Date: Fri, 13 Mar 2026 22:11:30 +0100
Subject: [PATCH 1/4] fix: improve sub-skill overwrite UX with content skip and
 collision protection

- Add content-identical skip: sub-skills with unchanged content are
  no longer removed/re-copied (uses filecmp byte-level comparison)
- Add user-authored collision protection: skills not in managed_files
  are skipped with actionable 'use --force' message instead of being
  silently destroyed
- Wire diagnostics to all integrate_package_skill() call sites so
  cross-package overwrites appear in the end-of-install summary
  instead of cluttering inline output
- Improve diagnostic wording from 'sub-skills overwrote existing
  skills' to 'skills replaced by a different package (last installed
  wins)'

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 .../content/docs/reference/cli-commands.md    |   2 +-
 src/apm_cli/commands/install.py               |  11 +-
 src/apm_cli/integration/skill_integrator.py   |  73 +++++-
 src/apm_cli/utils/diagnostics.py              |   7 +-
 .../unit/integration/test_skill_integrator.py | 210 ++++++++++++++++++
 tests/unit/test_diagnostics.py                |   4 +-
 6 files changed, 288 insertions(+), 19 deletions(-)

diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md
index 4cc7fbfd..b89c1877 100644
--- a/docs/src/content/docs/reference/cli-commands.md
+++ b/docs/src/content/docs/reference/cli-commands.md
@@ -208,7 +208,7 @@ When you run `apm install`, APM automatically integrates primitives from install
 
 **Diagnostic Summary:**
 
-After installation completes, APM prints a grouped diagnostic summary instead of inline warnings. Categories include collisions (skipped files), sub-skill overwrites, warnings, and errors.
+After installation completes, APM prints a grouped diagnostic summary instead of inline warnings. Categories include collisions (skipped files), cross-package skill replacements, warnings, and errors.
 
 - **Normal mode**: Shows counts and actionable tips (e.g., "9 files skipped -- use `apm install --force` to overwrite")
 - **Verbose mode** (`--verbose`): Additionally lists individual file paths grouped by package, and full error details
diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py
index 6b726572..f3881873 100644
--- a/src/apm_cli/commands/install.py
+++ b/src/apm_cli/commands/install.py
@@ -577,7 +577,10 @@ def _integrate_package_primitives(
 
     # --- skills ---
     if integrate_vscode or integrate_claude:
-        skill_result = skill_integrator.integrate_package_skill(package_info, project_root)
+        skill_result = skill_integrator.integrate_package_skill(
+            package_info, project_root,
+            diagnostics=diagnostics, managed_files=managed_files, force=force,
+        )
         if skill_result.skill_created:
             result["skills"] += 1
             _rich_info(f"  └─ Skill integrated → .github/skills/")
@@ -1365,7 +1368,8 @@ def _collect_descendants(node, visited=None):
                             # Skills go to .github/skills/ (primary) and .claude/skills/ (if .claude/ exists)
                             if integrate_vscode or integrate_claude:
                                 skill_result = skill_integrator.integrate_package_skill(
-                                    cached_package_info, project_root
+                                    cached_package_info, project_root,
+                                    diagnostics=diagnostics, managed_files=managed_files, force=force,
                                 )
                                 if skill_result.skill_created:
                                     total_skills_integrated += 1
@@ -1617,7 +1621,8 @@ def _collect_descendants(node, visited=None):
                             # Skills go to .github/skills/ (primary) and .claude/skills/ (if .claude/ exists)
                             if integrate_vscode or integrate_claude:
                                 skill_result = skill_integrator.integrate_package_skill(
-                                    package_info, project_root
+                                    package_info, project_root,
+                                    diagnostics=diagnostics, managed_files=managed_files, force=force,
                                 )
                                 if skill_result.skill_created:
                                     total_skills_integrated += 1
diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py
index 621649ba..aa5cacf0 100644
--- a/src/apm_cli/integration/skill_integrator.py
+++ b/src/apm_cli/integration/skill_integrator.py
@@ -4,6 +4,7 @@
 from typing import List, Dict
 from dataclasses import dataclass
 from datetime import datetime
+import filecmp
 import hashlib
 import shutil
 import re
@@ -436,7 +437,28 @@ def find_context_files(self, package_path: Path) -> List[Path]:
         return context_files
     
     @staticmethod
-    def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_name: str, *, warn: bool = True, owned_by: dict[str, str] | None = None, diagnostics=None) -> tuple[int, list[Path]]:
+    def _dirs_equal(dir_a: Path, dir_b: Path) -> bool:
+        """Check if two directory trees have identical file contents."""
+        dcmp = filecmp.dircmp(str(dir_a), str(dir_b))
+        return SkillIntegrator._dircmp_equal(dcmp)
+
+    @staticmethod
+    def _dircmp_equal(dcmp) -> bool:
+        """Recursively check if dircmp shows identical contents."""
+        if dcmp.left_only or dcmp.right_only or dcmp.funny_files:
+            return False
+        _, mismatches, errors = filecmp.cmpfiles(
+            dcmp.left, dcmp.right, dcmp.common_files, shallow=False
+        )
+        if mismatches or errors:
+            return False
+        for sub_dcmp in dcmp.subdirs.values():
+            if not SkillIntegrator._dircmp_equal(sub_dcmp):
+                return False
+        return True
+
+    @staticmethod
+    def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_name: str, *, warn: bool = True, owned_by: dict[str, str] | None = None, diagnostics=None, managed_files=None, force: bool = False) -> tuple[int, list[Path]]:
         """Promote sub-skills from .apm/skills/ to top-level skill entries.
 
         Args:
@@ -465,14 +487,46 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n
             sub_name = raw_sub_name if is_valid else normalize_skill_name(raw_sub_name)
             target = target_skills_root / sub_name
             if target.exists():
+                # Content-identical → skip entirely (no copy, no warning)
+                if SkillIntegrator._dirs_equal(sub_skill_path, target):
+                    promoted += 1
+                    deployed.append(target)
+                    continue
+
+                # 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
+
+                # Cross-package overwrite with different content
                 if warn and not is_self_overwrite:
                     if diagnostics is not None:
                         diagnostics.overwrite(
                             path=f"{target_skills_root.name}/{sub_name}/",
                             package=parent_name,
-                            detail=f"Sub-skill '{sub_name}' from '{parent_name}' overwrites existing skill",
+                            detail=f"Skill '{sub_name}' replaced — previously from another package",
                         )
                     else:
                         try:
@@ -512,6 +566,7 @@ def _build_skill_ownership_map(project_root: Path) -> dict[str, str]:
 
     def _promote_sub_skills_standalone(
         self, package_info, project_root: Path, diagnostics=None,
+        managed_files=None, force: bool = False,
     ) -> tuple[int, list[Path]]:
         """Promote sub-skills from a package that is NOT itself a skill.
 
@@ -537,7 +592,7 @@ def _promote_sub_skills_standalone(
         github_skills_root.mkdir(parents=True, exist_ok=True)
         owned_by = self._build_skill_ownership_map(project_root)
         count, deployed = self._promote_sub_skills(
-            sub_skills_dir, github_skills_root, parent_name, warn=True, owned_by=owned_by, diagnostics=diagnostics
+            sub_skills_dir, github_skills_root, parent_name, warn=True, owned_by=owned_by, diagnostics=diagnostics, managed_files=managed_files, force=force
         )
         all_deployed = list(deployed)
 
@@ -554,7 +609,7 @@ def _promote_sub_skills_standalone(
 
     def _integrate_native_skill(
         self, package_info, project_root: Path, source_skill_md: Path,
-        diagnostics=None,
+        diagnostics=None, managed_files=None, force: bool = False,
     ) -> SkillIntegrationResult:
         """Copy a native Skill (with existing SKILL.md) to .github/skills/ and optionally .claude/skills/.
         
@@ -645,7 +700,7 @@ def _integrate_native_skill(
         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)
+        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)
         all_target_paths.extend(sub_deployed)
         
         # === T7: Copy to .claude/skills/ (secondary - compatibility) ===
@@ -677,7 +732,7 @@ def _integrate_native_skill(
             target_paths=all_target_paths
         )
 
-    def integrate_package_skill(self, package_info, project_root: Path, diagnostics=None) -> SkillIntegrationResult:
+    def integrate_package_skill(self, package_info, project_root: Path, diagnostics=None, managed_files=None, force: bool = False) -> SkillIntegrationResult:
         """Integrate a package's skill into .github/skills/ directory.
         
         Copies native skills (packages with SKILL.md at root) to .github/skills/
@@ -700,7 +755,7 @@ def integrate_package_skill(self, package_info, project_root: Path, diagnostics=
             # Even non-skill packages may ship sub-skills under .apm/skills/.
             # Promote them so Copilot can discover them independently.
             sub_skills_count, sub_deployed = self._promote_sub_skills_standalone(
-                package_info, project_root, diagnostics=diagnostics
+                package_info, project_root, diagnostics=diagnostics, managed_files=managed_files, force=force
             )
             return SkillIntegrationResult(
                 skill_created=False,
@@ -733,12 +788,12 @@ def integrate_package_skill(self, package_info, project_root: Path, diagnostics=
         # Check if this is a native Skill (already has SKILL.md at root)
         source_skill_md = package_path / "SKILL.md"
         if source_skill_md.exists():
-            return self._integrate_native_skill(package_info, project_root, source_skill_md, diagnostics=diagnostics)
+            return self._integrate_native_skill(package_info, project_root, source_skill_md, diagnostics=diagnostics, managed_files=managed_files, force=force)
         
         # No SKILL.md at root  -- not a skill package.
         # Still promote any sub-skills shipped under .apm/skills/.
         sub_skills_count, sub_deployed = self._promote_sub_skills_standalone(
-            package_info, project_root, diagnostics=diagnostics
+            package_info, project_root, diagnostics=diagnostics, managed_files=managed_files, force=force
         )
         return SkillIntegrationResult(
             skill_created=False,
diff --git a/src/apm_cli/utils/diagnostics.py b/src/apm_cli/utils/diagnostics.py
index fcbed95a..102e2efd 100644
--- a/src/apm_cli/utils/diagnostics.py
+++ b/src/apm_cli/utils/diagnostics.py
@@ -193,9 +193,9 @@ def _render_collision_group(self, items: List[Diagnostic]) -> None:
 
     def _render_overwrite_group(self, items: List[Diagnostic]) -> None:
         count = len(items)
-        noun = "sub-skill" if count == 1 else "sub-skills"
+        noun = "skill" if count == 1 else "skills"
         _rich_warning(
-            f"  ⚠ {count} {noun} overwrote existing skills"
+            f"  ⚠ {count} {noun} replaced by a different package (last installed wins)"
         )
         if not self.verbose:
             _rich_info("    Run with --verbose to see details")
@@ -205,8 +205,7 @@ def _render_overwrite_group(self, items: List[Diagnostic]) -> None:
                 if pkg:
                     _rich_echo(f"    [{pkg}]", color="dim")
                 for d in diags:
-                    detail_str = f" — {d.detail}" if d.detail else ""
-                    _rich_echo(f"      └─ {d.message}{detail_str}", color="dim")
+                    _rich_echo(f"      └─ {d.message}", color="dim")
 
     def _render_warning_group(self, items: List[Diagnostic]) -> None:
         for d in items:
diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py
index 43e01926..7224f294 100644
--- a/tests/unit/integration/test_skill_integrator.py
+++ b/tests/unit/integration/test_skill_integrator.py
@@ -2371,3 +2371,213 @@ def test_sync_preserves_promoted_sub_skills_when_package_installed(self):
 
         assert result['files_removed'] == 0
         assert style_checker.exists()
+
+
+class TestSubSkillContentSkipAndCollisionProtection:
+    """Test content-identical skip, user-authored collision protection, and diagnostics routing."""
+
+    def setup_method(self):
+        self.temp_dir = tempfile.mkdtemp()
+        self.project_root = Path(self.temp_dir)
+        self.integrator = SkillIntegrator()
+
+    def teardown_method(self):
+        shutil.rmtree(self.temp_dir, ignore_errors=True)
+
+    def _create_package_info(
+        self,
+        name: str = "test-pkg",
+        install_path: Path = None,
+        package_type: PackageType = PackageType.CLAUDE_SKILL
+    ) -> PackageInfo:
+        package = APMPackage(
+            name=name,
+            version="1.0.0",
+            package_path=install_path or self.project_root / "package",
+            source=f"github.com/test/{name}"
+        )
+        resolved_ref = ResolvedReference(
+            original_ref="main",
+            ref_type=GitReferenceType.BRANCH,
+            resolved_commit="abc123",
+            ref_name="main"
+        )
+        return PackageInfo(
+            package=package,
+            install_path=install_path or self.project_root / "package",
+            resolved_reference=resolved_ref,
+            installed_at=datetime.now().isoformat(),
+            package_type=package_type
+        )
+
+    def _create_package_with_sub_skills(self, name="parent-skill", sub_skills=None):
+        package_dir = self.project_root / name
+        package_dir.mkdir()
+        (package_dir / "SKILL.md").write_text(f"---\nname: {name}\ndescription: Parent skill\n---\n# {name}\n")
+        if sub_skills:
+            skills_dir = package_dir / ".apm" / "skills"
+            skills_dir.mkdir(parents=True)
+            for sub_name in sub_skills:
+                sub_dir = skills_dir / sub_name
+                sub_dir.mkdir()
+                (sub_dir / "SKILL.md").write_text(
+                    f"---\nname: {sub_name}\ndescription: Sub-skill {sub_name}\n---\n# {sub_name}\n"
+                )
+        return package_dir
+
+    def test_content_identical_sub_skill_skipped(self):
+        """When source and target sub-skill directories have identical content, skip the copy."""
+        package_dir = self._create_package_with_sub_skills("pkg", sub_skills=["my-skill"])
+        pkg_info = self._create_package_info(name="pkg", install_path=package_dir)
+
+        # First install
+        self.integrator.integrate_package_skill(pkg_info, self.project_root)
+        target = self.project_root / ".github" / "skills" / "my-skill" / "SKILL.md"
+        assert target.exists()
+
+        # Record mtime after first install
+        first_mtime = target.stat().st_mtime
+
+        import time
+        time.sleep(0.05)  # Ensure mtime would differ if file was rewritten
+
+        # Second install — content identical
+        self.integrator.integrate_package_skill(pkg_info, self.project_root)
+
+        # File should NOT have been rewritten (mtime unchanged)
+        assert target.stat().st_mtime == first_mtime
+
+    def test_content_different_sub_skill_replaced(self):
+        """When sub-skill content differs, it should be replaced."""
+        package_dir = self._create_package_with_sub_skills("pkg", sub_skills=["my-skill"])
+        pkg_info = self._create_package_info(name="pkg", install_path=package_dir)
+
+        # First install
+        self.integrator.integrate_package_skill(pkg_info, self.project_root)
+
+        # Modify the deployed skill to simulate drift
+        target = self.project_root / ".github" / "skills" / "my-skill" / "SKILL.md"
+        target.write_text("# Modified by user")
+
+        # Second install — content differs
+        self.integrator.integrate_package_skill(pkg_info, self.project_root)
+
+        # Should be overwritten with original content
+        content = target.read_text()
+        assert "Sub-skill my-skill" in content
+        assert "Modified by user" not in content
+
+    def test_user_authored_skill_skipped_without_force(self):
+        """User-authored skills (not in managed_files) should be skipped without --force."""
+        # Create a user-authored skill at the target path
+        user_skill = self.project_root / ".github" / "skills" / "my-skill"
+        user_skill.mkdir(parents=True)
+        (user_skill / "SKILL.md").write_text("# User authored skill")
+
+        # Create package that would deploy a sub-skill with the same name
+        package_dir = self._create_package_with_sub_skills("pkg", sub_skills=["my-skill"])
+        pkg_info = self._create_package_info(name="pkg", install_path=package_dir)
+
+        # managed_files is set but does NOT contain this skill → user-authored
+        managed_files = set()
+
+        from apm_cli.utils.diagnostics import DiagnosticCollector
+        diag = DiagnosticCollector()
+
+        self.integrator.integrate_package_skill(
+            pkg_info, self.project_root,
+            diagnostics=diag, managed_files=managed_files, force=False,
+        )
+
+        # User content should be preserved
+        content = (user_skill / "SKILL.md").read_text()
+        assert content == "# User authored skill"
+
+        # Diagnostic should record a collision skip
+        assert diag.has_diagnostics
+        groups = diag.by_category()
+        from apm_cli.utils.diagnostics import CATEGORY_COLLISION
+        assert CATEGORY_COLLISION in groups
+        assert any("my-skill" in d.message for d in groups[CATEGORY_COLLISION])
+
+    def test_user_authored_skill_overwritten_with_force(self):
+        """User-authored skills should be overwritten when force=True."""
+        user_skill = self.project_root / ".github" / "skills" / "my-skill"
+        user_skill.mkdir(parents=True)
+        (user_skill / "SKILL.md").write_text("# User authored skill")
+
+        package_dir = self._create_package_with_sub_skills("pkg", sub_skills=["my-skill"])
+        pkg_info = self._create_package_info(name="pkg", install_path=package_dir)
+
+        managed_files = set()  # Not managed
+
+        self.integrator.integrate_package_skill(
+            pkg_info, self.project_root,
+            managed_files=managed_files, force=True,
+        )
+
+        # Should be overwritten
+        content = (user_skill / "SKILL.md").read_text()
+        assert "Sub-skill my-skill" in content
+        assert "User authored" not in content
+
+    def test_cross_package_overwrite_records_diagnostic(self):
+        """Cross-package overwrites should record a diagnostic, not print inline."""
+        # Pre-existing managed skill from a different package
+        existing = self.project_root / ".github" / "skills" / "shared-skill"
+        existing.mkdir(parents=True)
+        (existing / "SKILL.md").write_text("# From other-pkg")
+
+        package_dir = self._create_package_with_sub_skills("my-pkg", sub_skills=["shared-skill"])
+        pkg_info = self._create_package_info(name="my-pkg", install_path=package_dir)
+
+        # Managed files includes this skill → it's APM-managed
+        managed_files = {".github/skills/shared-skill"}
+
+        from apm_cli.utils.diagnostics import DiagnosticCollector, CATEGORY_OVERWRITE
+        diag = DiagnosticCollector()
+
+        with patch("apm_cli.utils.console._rich_warning") as mock_warning:
+            self.integrator.integrate_package_skill(
+                pkg_info, self.project_root,
+                diagnostics=diag, managed_files=managed_files, force=False,
+            )
+
+        # Should NOT have printed an inline warning
+        mock_warning.assert_not_called()
+
+        # Should have recorded an overwrite diagnostic
+        assert diag.has_diagnostics
+        groups = diag.by_category()
+        assert CATEGORY_OVERWRITE in groups
+        assert any("shared-skill" in d.message for d in groups[CATEGORY_OVERWRITE])
+
+    def test_self_overwrite_silent_no_diagnostic(self):
+        """Self-overwrites (same package re-deploys) with different content should be silent."""
+        package_dir = self._create_package_with_sub_skills("my-pkg", sub_skills=["my-sub"])
+        pkg_info = self._create_package_info(name="my-pkg", install_path=package_dir)
+
+        # First install
+        self.integrator.integrate_package_skill(pkg_info, self.project_root)
+
+        # Modify deployed content to force a non-identical state
+        target = self.project_root / ".github" / "skills" / "my-sub" / "SKILL.md"
+        target.write_text("# Drifted content")
+
+        # Create mock lockfile that records ownership by my-pkg
+        managed_files = {".github/skills/my-sub"}
+        from apm_cli.utils.diagnostics import DiagnosticCollector
+        diag = DiagnosticCollector()
+
+        with patch.object(SkillIntegrator, '_build_skill_ownership_map', return_value={"my-sub": "my-pkg"}):
+            self.integrator.integrate_package_skill(
+                pkg_info, self.project_root,
+                diagnostics=diag, managed_files=managed_files, force=False,
+            )
+
+        # Self-overwrite — no diagnostics should be recorded
+        assert not diag.has_diagnostics
+
+        # Content should be updated
+        content = target.read_text()
+        assert "Sub-skill my-sub" in content
diff --git a/tests/unit/test_diagnostics.py b/tests/unit/test_diagnostics.py
index a0c2ea92..1f5b4e04 100644
--- a/tests/unit/test_diagnostics.py
+++ b/tests/unit/test_diagnostics.py
@@ -230,7 +230,7 @@ def test_overwrite_group_shows_overwrote_message(
         dc.overwrite("skill.md", package="pkg")
         dc.render_summary()
         warning_texts = [str(c) for c in mock_warning.call_args_list]
-        assert any("sub-skill" in t and "overwrote" in t for t in warning_texts)
+        assert any("skill" in t and "replaced" in t for t in warning_texts)
 
     @patch(f"{_MOCK_BASE}._get_console", return_value=None)
     @patch(f"{_MOCK_BASE}._rich_echo")
@@ -282,7 +282,7 @@ def test_render_summary_handles_all_categories(
         combined = " ".join(all_texts)
         # All categories should appear
         assert "skipped" in combined
-        assert "overwrote" in combined
+        assert "replaced" in combined
         assert "watch out" in combined
         assert "failed" in combined
 

From c5126545b85a9b8e9e90ff772ccbd7d116a2972c Mon Sep 17 00:00:00 2001
From: danielmeppiel <dmeppiel@microsoft.com>
Date: Fri, 13 Mar 2026 22:30:07 +0100
Subject: [PATCH 2/4] feat: add cli-logging-ux agent skill for CLI output
 conventions

Codifies DX principles for CLI logging: Traffic Light Rule, So What
Test, Newspaper Test, DiagnosticCollector usage, console helper
conventions, and anti-patterns. Activates when editing user-facing
terminal output code.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 .github/skills/cli-logging-ux/SKILL.md | 158 +++++++++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100644 .github/skills/cli-logging-ux/SKILL.md

diff --git a/.github/skills/cli-logging-ux/SKILL.md b/.github/skills/cli-logging-ux/SKILL.md
new file mode 100644
index 00000000..8551941e
--- /dev/null
+++ b/.github/skills/cli-logging-ux/SKILL.md
@@ -0,0 +1,158 @@
+---
+name: cli-logging-ux
+description: >
+  Use this skill when editing or creating CLI output, logging, warnings,
+  error messages, progress indicators, or diagnostic summaries in the APM
+  codebase. Activate whenever code touches console helpers (_rich_success,
+  _rich_warning, _rich_error, _rich_info, _rich_echo), DiagnosticCollector,
+  STATUS_SYMBOLS, or any user-facing terminal output — even if the user
+  doesn't mention "logging" or "UX" explicitly.
+---
+
+# CLI Logging & Developer Experience
+
+## Decision framework
+
+Apply these three tests to every piece of user-facing output. If a message fails any test, redesign it.
+
+### 1. The "So What?" Test
+
+Every warning must answer: *what should the user do about this?*
+
+```
+# Fails — not actionable, user can't do anything
+Sub-skill 'my-skill' from 'my-package' overwrites existing skill
+
+# Passes — tells the user exactly what to do
+Skipping my-skill — local file exists (not managed by APM). Use 'apm install --force' to overwrite.
+```
+
+If the user can't act on it, it's not a warning — it's noise. Demote to `--verbose` or remove.
+
+### 2. The Traffic Light Rule
+
+Use color semantics consistently. Never use a warning color for an informational state.
+
+| Color | Helper | Meaning | When to use |
+|-------|--------|---------|-------------|
+| Green | `_rich_success()` | Success / completed | Operation finished as expected |
+| Yellow | `_rich_warning()` | User action needed | Something requires user decision |
+| Red | `_rich_error()` | Error / failure | Operation failed, cannot continue |
+| Blue | `_rich_info()` | Informational | Status updates, progress, summaries |
+| Dim | `_rich_echo(color="dim")` | Secondary detail | Verbose-mode details, grouping headers |
+
+### 3. The Newspaper Test
+
+Can the user scan output like headlines? Top-level = what happened. Details = drill down.
+
+```
+# Bad — warnings break the visual flow between status and summary
+[checkmark] package-name
+[warning] something happened
+[warning] something else happened
+  [tree] 3 skill(s) integrated
+
+# Good — clean tree, diagnostics at the end
+[checkmark] package-name
+  [tree] 3 skill(s) integrated
+
+── Diagnostics ──
+  [warning] 2 skills replaced by a different package (last installed wins)
+    Run with --verbose to see details
+```
+
+## Inline output vs deferred diagnostics
+
+### Use inline output for:
+- Success confirmations (`_rich_success`)
+- Progress updates (`_rich_info` with indented `└─` prefix)
+- Errors that halt the current operation (`_rich_error`)
+
+### Use DiagnosticCollector for:
+- Warnings that apply across multiple packages (collisions, overwrites)
+- Issues the user should know about but that don't stop the operation
+- Anything that would repeat N times in a loop
+
+```python
+# Bad — inline warning repeated per file, clutters output
+for file in files:
+    if collision:
+        _rich_warning(f"Skipping {file}...")
+
+# Good — collect during loop, render grouped summary at the end
+for file in files:
+    if collision:
+        diagnostics.skip(file, package=pkg_name)
+
+# Later, after the loop:
+if diagnostics.has_diagnostics:
+    diagnostics.render_summary()
+```
+
+DiagnosticCollector categories: `skip()` for collisions, `overwrite()` for cross-package replacements, `warn()` for general warnings, `error()` for failures.
+
+## Console helper conventions
+
+Always use the helpers from `apm_cli.utils.console` — never raw `print()` or bare `click.echo()`.
+
+```python
+from apm_cli.utils.console import (
+    _rich_success, _rich_error, _rich_warning, _rich_info, _rich_echo
+)
+
+_rich_success("Installed 3 APM dependencies")        # green, bold
+_rich_info("  └─ 2 prompts integrated → .github/prompts/")  # blue
+_rich_warning("Config drift detected — re-run apm install")  # yellow
+_rich_error("Failed to download package")              # red
+_rich_echo("    [pkg-name]", color="dim")              # dim, for verbose details
+```
+
+Use `STATUS_SYMBOLS` dict with `symbol=` parameter instead of hardcoding emojis:
+```python
+_rich_info("Starting operation...", symbol="gear")     # uses STATUS_SYMBOLS["gear"]
+```
+
+## Output structure pattern
+
+Follow this visual hierarchy for multi-package operations:
+
+```
+[checkmark] package-name-1                      # _rich_success — download/copy ok
+  [tree] 2 prompts integrated → .github/prompts/     # _rich_info — indented summary
+  [tree] 1 skill(s) integrated → .github/skills/
+[checkmark] package-name-2
+  [tree] 1 instruction(s) integrated → .github/instructions/
+
+── Diagnostics ──                         # Only if diagnostics.has_diagnostics
+  [warning] N files skipped — ...                   # Grouped by category
+    Run with --verbose to see details
+
+Installed 2 APM dependencies              # _rich_success — final summary
+```
+
+## Content-awareness principle
+
+Before reporting changes, check if anything actually changed. Don't report no-ops.
+
+```python
+# Bad — always copies and reports, even when content is identical
+shutil.rmtree(target)
+shutil.copytree(source, target)
+_rich_info(f"  └─ Skill updated")
+
+# Good — skip when content matches
+if SkillIntegrator._dirs_equal(source, target):
+    continue  # Nothing changed, nothing to report
+```
+
+## Anti-patterns
+
+1. **Warning for non-actionable state** — If the user can't do anything about it, use `_rich_info` or defer to `--verbose`, not `_rich_warning`.
+
+2. **Inline warnings in loops** — Use `DiagnosticCollector` to collect, then render a grouped summary after the loop.
+
+3. **Missing `diagnostics` parameter** — When calling integrators, always pass `diagnostics=diagnostics` so warnings route to the deferred summary.
+
+4. **Inconsistent symbols** — Use `STATUS_SYMBOLS` dict, not hardcoded emoji characters.
+
+5. **Walls of text** — Use Rich tables for structured data, panels for grouped content. Break up long output with visual hierarchy (indentation, `└─` tree connectors).

From b6e848e5489c4f9ed17600c1f9fb19f3f9d7a224 Mon Sep 17 00:00:00 2001
From: danielmeppiel <dmeppiel@microsoft.com>
Date: Fri, 13 Mar 2026 22:46:42 +0100
Subject: [PATCH 3/4] fix: address PR review comments on skill overwrite UX

- Render diagnostic detail in verbose mode for overwrite group
  (previously detail was accepted but never displayed)
- Use project-relative paths in diagnostic messages
  (.github/skills/name instead of skills/name)
- Replace flaky mtime-based test with mock-based assertion
- Add test for verbose detail rendering

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 src/apm_cli/integration/skill_integrator.py   | 31 +++++++++++++------
 src/apm_cli/utils/diagnostics.py              |  2 ++
 .../unit/integration/test_skill_integrator.py | 20 ++++++------
 tests/unit/test_diagnostics.py                | 13 ++++++++
 4 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/src/apm_cli/integration/skill_integrator.py b/src/apm_cli/integration/skill_integrator.py
index aa5cacf0..4cf580c9 100644
--- a/src/apm_cli/integration/skill_integrator.py
+++ b/src/apm_cli/integration/skill_integrator.py
@@ -458,7 +458,7 @@ def _dircmp_equal(dcmp) -> bool:
         return True
 
     @staticmethod
-    def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_name: str, *, warn: bool = True, owned_by: dict[str, str] | None = None, diagnostics=None, managed_files=None, force: bool = False) -> tuple[int, list[Path]]:
+    def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_name: str, *, warn: bool = True, owned_by: dict[str, str] | None = None, diagnostics=None, managed_files=None, force: bool = False, project_root: Path | None = None) -> tuple[int, list[Path]]:
         """Promote sub-skills from .apm/skills/ to top-level skill entries.
 
         Args:
@@ -469,6 +469,7 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n
             owned_by: Map of skill_name -> owner_package_name from the lockfile.
                 When provided, warnings are suppressed for self-overwrites.
             diagnostics: Optional DiagnosticCollector for deferred warning output.
+            project_root: Project root for computing relative diagnostic paths.
 
         Returns:
             tuple[int, list[Path]]: (count of promoted sub-skills, list of deployed dir paths)
@@ -477,6 +478,16 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n
         deployed = []
         if not sub_skills_dir.is_dir():
             return promoted, deployed
+
+        # Compute project-relative prefix for consistent path reporting
+        if project_root is not None:
+            try:
+                rel_prefix = target_skills_root.relative_to(project_root).as_posix()
+            except ValueError:
+                rel_prefix = target_skills_root.name
+        else:
+            rel_prefix = target_skills_root.name
+
         for sub_skill_path in sub_skills_dir.iterdir():
             if not sub_skill_path.is_dir():
                 continue
@@ -486,6 +497,7 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n
             is_valid, _ = validate_skill_name(raw_sub_name)
             sub_name = raw_sub_name if is_valid else normalize_skill_name(raw_sub_name)
             target = target_skills_root / sub_name
+            rel_path = f"{rel_prefix}/{sub_name}"
             if target.exists():
                 # Content-identical → skip entirely (no copy, no warning)
                 if SkillIntegrator._dirs_equal(sub_skill_path, target):
@@ -494,10 +506,9 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n
                     continue
 
                 # 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
+                    and rel_path.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
@@ -507,7 +518,7 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n
                     if not force:
                         if diagnostics is not None:
                             diagnostics.skip(
-                                f"skills/{sub_name}/", package=parent_name
+                                rel_path, package=parent_name
                             )
                         else:
                             try:
@@ -524,7 +535,7 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n
                 if warn and not is_self_overwrite:
                     if diagnostics is not None:
                         diagnostics.overwrite(
-                            path=f"{target_skills_root.name}/{sub_name}/",
+                            path=rel_path,
                             package=parent_name,
                             detail=f"Skill '{sub_name}' replaced — previously from another package",
                         )
@@ -532,7 +543,7 @@ def _promote_sub_skills(sub_skills_dir: Path, target_skills_root: Path, parent_n
                         try:
                             from apm_cli.utils.console import _rich_warning
                             _rich_warning(
-                                f"Sub-skill '{sub_name}' from '{parent_name}' overwrites existing skill at {target_skills_root.name}/{sub_name}/"
+                                f"Sub-skill '{sub_name}' from '{parent_name}' overwrites existing skill at {rel_path}"
                             )
                         except ImportError:
                             pass
@@ -592,7 +603,7 @@ def _promote_sub_skills_standalone(
         github_skills_root.mkdir(parents=True, exist_ok=True)
         owned_by = self._build_skill_ownership_map(project_root)
         count, deployed = self._promote_sub_skills(
-            sub_skills_dir, github_skills_root, parent_name, warn=True, owned_by=owned_by, diagnostics=diagnostics, managed_files=managed_files, force=force
+            sub_skills_dir, github_skills_root, parent_name, warn=True, owned_by=owned_by, diagnostics=diagnostics, managed_files=managed_files, force=force, project_root=project_root
         )
         all_deployed = list(deployed)
 
@@ -601,7 +612,7 @@ def _promote_sub_skills_standalone(
         if claude_dir.exists() and claude_dir.is_dir():
             claude_skills_root = claude_dir / "skills"
             _, claude_deployed = self._promote_sub_skills(
-                sub_skills_dir, claude_skills_root, parent_name, warn=False
+                sub_skills_dir, claude_skills_root, parent_name, warn=False, project_root=project_root
             )
             all_deployed.extend(claude_deployed)
 
@@ -700,7 +711,7 @@ def _integrate_native_skill(
         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)
+        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, project_root=project_root)
         all_target_paths.extend(sub_deployed)
         
         # === T7: Copy to .claude/skills/ (secondary - compatibility) ===
@@ -718,7 +729,7 @@ def _integrate_native_skill(
             
             # Promote sub-skills for Claude too
             claude_skills_root = claude_dir / "skills"
-            _, claude_sub_deployed = self._promote_sub_skills(sub_skills_dir, claude_skills_root, skill_name, warn=False)
+            _, claude_sub_deployed = self._promote_sub_skills(sub_skills_dir, claude_skills_root, skill_name, warn=False, project_root=project_root)
             all_target_paths.extend(claude_sub_deployed)
         
         return SkillIntegrationResult(
diff --git a/src/apm_cli/utils/diagnostics.py b/src/apm_cli/utils/diagnostics.py
index 102e2efd..96ec9441 100644
--- a/src/apm_cli/utils/diagnostics.py
+++ b/src/apm_cli/utils/diagnostics.py
@@ -206,6 +206,8 @@ def _render_overwrite_group(self, items: List[Diagnostic]) -> None:
                     _rich_echo(f"    [{pkg}]", color="dim")
                 for d in diags:
                     _rich_echo(f"      └─ {d.message}", color="dim")
+                    if d.detail:
+                        _rich_echo(f"         {d.detail}", color="dim")
 
     def _render_warning_group(self, items: List[Diagnostic]) -> None:
         for d in items:
diff --git a/tests/unit/integration/test_skill_integrator.py b/tests/unit/integration/test_skill_integrator.py
index 7224f294..ac00dc1a 100644
--- a/tests/unit/integration/test_skill_integrator.py
+++ b/tests/unit/integration/test_skill_integrator.py
@@ -2435,17 +2435,15 @@ def test_content_identical_sub_skill_skipped(self):
         target = self.project_root / ".github" / "skills" / "my-skill" / "SKILL.md"
         assert target.exists()
 
-        # Record mtime after first install
-        first_mtime = target.stat().st_mtime
-
-        import time
-        time.sleep(0.05)  # Ensure mtime would differ if file was rewritten
-
-        # Second install — content identical
-        self.integrator.integrate_package_skill(pkg_info, self.project_root)
-
-        # File should NOT have been rewritten (mtime unchanged)
-        assert target.stat().st_mtime == first_mtime
+        # Second install — content identical; copytree/rmtree should NOT be called
+        from unittest.mock import patch
+        with patch("shutil.rmtree") as mock_rm, patch("shutil.copytree") as mock_cp:
+            self.integrator.integrate_package_skill(pkg_info, self.project_root)
+            # Neither rmtree nor copytree should be invoked for the identical sub-skill
+            for call in mock_rm.call_args_list:
+                assert "my-skill" not in str(call), "rmtree called on identical sub-skill"
+            for call in mock_cp.call_args_list:
+                assert "my-skill" not in str(call), "copytree called on identical sub-skill"
 
     def test_content_different_sub_skill_replaced(self):
         """When sub-skill content differs, it should be replaced."""
diff --git a/tests/unit/test_diagnostics.py b/tests/unit/test_diagnostics.py
index 1f5b4e04..0c9d6198 100644
--- a/tests/unit/test_diagnostics.py
+++ b/tests/unit/test_diagnostics.py
@@ -232,6 +232,19 @@ def test_overwrite_group_shows_overwrote_message(
         warning_texts = [str(c) for c in mock_warning.call_args_list]
         assert any("skill" in t and "replaced" in t for t in warning_texts)
 
+    @patch(f"{_MOCK_BASE}._get_console", return_value=None)
+    @patch(f"{_MOCK_BASE}._rich_echo")
+    @patch(f"{_MOCK_BASE}._rich_warning")
+    @patch(f"{_MOCK_BASE}._rich_info")
+    def test_overwrite_verbose_renders_detail(
+        self, mock_info, mock_warning, mock_echo, mock_console
+    ):
+        dc = DiagnosticCollector(verbose=True)
+        dc.overwrite("skill.md", package="pkg", detail="replaced by newer version")
+        dc.render_summary()
+        echo_texts = [str(c) for c in mock_echo.call_args_list]
+        assert any("replaced by newer version" in t for t in echo_texts)
+
     @patch(f"{_MOCK_BASE}._get_console", return_value=None)
     @patch(f"{_MOCK_BASE}._rich_echo")
     @patch(f"{_MOCK_BASE}._rich_warning")

From fceb793cec8fdc59eafecd03f0c252383aa8ccd0 Mon Sep 17 00:00:00 2001
From: danielmeppiel <dmeppiel@microsoft.com>
Date: Fri, 13 Mar 2026 22:56:50 +0100
Subject: [PATCH 4/4] fix: enforce absolute emoji ban in CLI logging skill

- Add explicit 'Emojis are banned' rule to console helper section
- Clarify STATUS_SYMBOLS renders ASCII text symbols, not emojis
- Add anti-pattern #4 for emoji ban enforcement

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 .github/skills/cli-logging-ux/SKILL.md | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/.github/skills/cli-logging-ux/SKILL.md b/.github/skills/cli-logging-ux/SKILL.md
index 8551941e..a7987936 100644
--- a/.github/skills/cli-logging-ux/SKILL.md
+++ b/.github/skills/cli-logging-ux/SKILL.md
@@ -95,6 +95,8 @@ DiagnosticCollector categories: `skip()` for collisions, `overwrite()` for cross
 
 Always use the helpers from `apm_cli.utils.console` — never raw `print()` or bare `click.echo()`.
 
+**Emojis are banned.** Never use emoji characters anywhere in CLI output — not in messages, symbols, help text, or status indicators. Use ASCII text symbols exclusively via `STATUS_SYMBOLS`.
+
 ```python
 from apm_cli.utils.console import (
     _rich_success, _rich_error, _rich_warning, _rich_info, _rich_echo
@@ -107,9 +109,9 @@ _rich_error("Failed to download package")              # red
 _rich_echo("    [pkg-name]", color="dim")              # dim, for verbose details
 ```
 
-Use `STATUS_SYMBOLS` dict with `symbol=` parameter instead of hardcoding emojis:
+Use `STATUS_SYMBOLS` dict with `symbol=` parameter for consistent ASCII prefixes:
 ```python
-_rich_info("Starting operation...", symbol="gear")     # uses STATUS_SYMBOLS["gear"]
+_rich_info("Starting operation...", symbol="gear")     # renders as "[*] Starting operation..."
 ```
 
 ## Output structure pattern
@@ -153,6 +155,8 @@ if SkillIntegrator._dirs_equal(source, target):
 
 3. **Missing `diagnostics` parameter** — When calling integrators, always pass `diagnostics=diagnostics` so warnings route to the deferred summary.
 
-4. **Inconsistent symbols** — Use `STATUS_SYMBOLS` dict, not hardcoded emoji characters.
+4. **No emojis, ever** — Emojis are completely banned from all CLI output. Use ASCII text symbols from `STATUS_SYMBOLS` exclusively. This applies to messages, help text, status indicators, and table titles.
+
+5. **Inconsistent symbols** — Always use `STATUS_SYMBOLS` dict with `symbol=` param, not inline characters.
 
-5. **Walls of text** — Use Rich tables for structured data, panels for grouped content. Break up long output with visual hierarchy (indentation, `└─` tree connectors).
+6. **Walls of text** — Use Rich tables for structured data, panels for grouped content. Break up long output with visual hierarchy (indentation, `└─` tree connectors).