feat: add DiagnosticCollector for structured install diagnostics by danielmeppiel · Pull Request #267 · microsoft/apm

danielmeppiel · 2026-03-12T20:52:15Z

Summary

Introduces a DiagnosticCollector pattern that collects warnings and errors during apm install and renders a clean, grouped summary at the end — instead of printing inline noise that clutters the install output.

What changed

Area	Details
Core	New `DiagnosticCollector` in `src/apm_cli/utils/diagnostics.py` — category-specific recording (skip, overwrite, warn, error), grouped rendering, thread-safe
Integrators	All 7 integrators accept optional `diagnostics=None` parameter (backward compatible)
Install wiring	`install.py` creates collector, passes to 18 call sites, routes 3 exception handlers, renders summary before install summary
Tests	28 new unit tests for DiagnosticCollector + 7 integrator collision tests
Docs	CLI reference and dependency guide updated

How it looks

Normal mode — concise counts:

── Install Diagnostics ──────────────────────
  2 file(s) skipped (already exist, use --force to overwrite)
  1 warning(s)

Verbose mode (--verbose) — full details grouped by package:

── Install Diagnostics ──────────────────────
  Skipped files (use --force to overwrite):
    └─ package-a
       └─ .github/prompts/foo.md
       └─ .github/prompts/bar.md
  Warnings:
    └─ package-b
       └─ Could not parse config file

Test results

1754 passed (1 pre-existing failure on main excluded)

Introduce a DiagnosticCollector pattern that collects warnings and errors during 'apm install' and renders a clean, grouped summary at the end instead of printing inline noise. Changes: - Add DiagnosticCollector (src/apm_cli/utils/diagnostics.py) with category-specific recording (skip, overwrite, warn, error), grouped rendering (normal counts vs verbose details), and thread safety - Update BaseIntegrator.check_collision() to accept optional diagnostics parameter — routes through collector when present, _rich_warning() fallback when absent (backward compatible) - Update all integrator integrate_package_*() methods with optional diagnostics parameter forwarded to check_collision() - Wire DiagnosticCollector into install.py — create at start, pass to all 18 integrator call sites, route exception handlers through collector, render summary before install summary - Add 28 unit tests for DiagnosticCollector covering recording, query helpers, rendering, thread safety, and grouping - Add 7 integrator tests verifying collector vs fallback behavior - Update docs for install diagnostic summary and --verbose behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Adds a structured “collect then summarize” diagnostics flow for apm install, so collisions/warnings/errors can be recorded during integration and rendered as a grouped summary at the end of the install.

Changes:

Introduces DiagnosticCollector/Diagnostic and category constants, plus grouped rendering logic.
Threads an optional diagnostics parameter through integrators and BaseIntegrator.check_collision(), and wires a collector into the install engine.
Adds unit/integration test coverage and updates CLI/dependencies documentation to describe the new diagnostic summary behavior.

Reviewed changes

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

Show a summary per file

File	Description
tests/unit/test_install_command.py	Updates mocks for `_install_apm_dependencies()` to return the new diagnostics object.
tests/unit/test_diagnostics.py	Adds unit tests for recording, querying, rendering, grouping, and thread-safety behavior of `DiagnosticCollector`.
tests/unit/integration/test_deployed_files_manifest.py	Updates collision-output assertions and adds tests for `BaseIntegrator.check_collision(..., diagnostics=...)`.
src/apm_cli/utils/diagnostics.py	New diagnostics collector implementation (recording, grouping, summary rendering).
src/apm_cli/utils/init.py	Exposes diagnostics types/constants via `apm_cli.utils`.
src/apm_cli/integration/skill_integrator.py	Adds optional diagnostics support for sub-skill overwrite and skill-name normalization warnings.
src/apm_cli/integration/prompt_integrator.py	Passes diagnostics through collision checks for prompts.
src/apm_cli/integration/instruction_integrator.py	Passes diagnostics through collision checks for instructions.
src/apm_cli/integration/hook_integrator.py	Passes diagnostics through collision checks for hooks and hook scripts.
src/apm_cli/integration/command_integrator.py	Passes diagnostics through collision checks for commands.
src/apm_cli/integration/base_integrator.py	Adds `diagnostics` parameter to `check_collision()` and routes collision reporting to diagnostics vs `_rich_warning`.
src/apm_cli/integration/agent_integrator.py	Passes diagnostics through collision checks for agents (both `.github` and `.claude`).
src/apm_cli/commands/install.py	Instantiates a collector, passes it into multiple integrator calls, records install/integration failures into diagnostics, and renders the summary at the end.
docs/src/content/docs/reference/cli-commands.md	Updates `--verbose` description and documents the new diagnostic summary behavior for `apm install`.
docs/src/content/docs/guides/dependencies.md	Adds troubleshooting guidance for file conflicts and clarifies `apm install --verbose` behavior.

Comments suppressed due to low confidence (1)

src/apm_cli/commands/install.py:1214

Same issue on this code path: diagnostics are passed into prompt/agent/etc., but the corresponding integrate_package_skill() invocation in the same try-block does not receive the collector. Please pass diagnostics=diagnostics there as well so skill-related diagnostics are captured consistently.

                                prompt_integrator.integrate_package_prompts(
                                    package_info, project_root,
                                    force=force, managed_files=managed_files,
                                    diagnostics=diagnostics,
                                )
                            )
                            if prompt_result.files_integrated > 0:

Copilot · 2026-03-12T20:57:07Z

src/apm_cli/integration/base_integrator.py

@@ -75,11 +77,13 @@ def check_collision(
        if force:
            return False

-        print(
-            f"\u26a0\ufe0f  Skipping {rel_path} \u2014 local file exists (not managed by APM). "
-            f"Use 'apm install --force' to overwrite.",
-            file=sys.stderr,
-        )
+        if diagnostics is not None:
+            diagnostics.skip(rel_path)
+        else:
+            _rich_warning(
+                f"Skipping {rel_path} — local file exists (not managed by APM). "
+                f"Use 'apm install --force' to overwrite."
+            )


When routing collisions into diagnostics, the recorded entry does not include the package name. In verbose mode, _render_collision_group() groups by package, so collisions collected via check_collision() will all fall under the empty-package bucket and won’t match the "grouped by package" behavior described in the PR. Consider extending check_collision() to accept an optional package (or package_name) and pass it from integrators (e.g., package_info.package.name).

Copilot · 2026-03-12T20:57:08Z

src/apm_cli/integration/skill_integrator.py

+                diagnostics.warn(
+                    f"Skill name '{raw_skill_name}' normalized to '{skill_name}' ({error_msg})",
+                    package=raw_skill_name,


The diagnostic is recorded with package=raw_skill_name (the on-disk folder name), which may not match the package identifier used elsewhere in the install diagnostics (typically package_info.package.name / repo key). This will make grouping inconsistent. Consider using the actual package name/key for the package field, and keep the raw/normalized skill name in the message/detail instead.

Suggested change

diagnostics.warn(

f"Skill name '{raw_skill_name}' normalized to '{skill_name}' ({error_msg})",

package=raw_skill_name,

# Use the canonical package identifier for diagnostic grouping,

# and keep the raw/normalized skill names in the message.

package_name = getattr(

getattr(package_info, "package", None),

"name",

raw_skill_name,

)

diagnostics.warn(

f"Skill name '{raw_skill_name}' normalized to '{skill_name}' "

f"for package '{package_name}' ({error_msg})",

package=package_name,

Copilot · 2026-03-12T20:57:08Z

tests/unit/integration/test_deployed_files_manifest.py

    """Verify collision detection emits warning message to stderr."""

    def test_prompt_collision_warns_on_stderr(self, tmp_path: Path, capsys):
-        """Prompt collision should print warning to stderr."""
+        """Prompt collision should print warning via _rich_warning."""


This test class/docstring still says collision warnings are emitted to stderr, but the assertions now accept stdout or stderr (and _rich_warning typically writes to stdout). Update the docstring/class name to match the new behavior so the test intent stays accurate.

Copilot · 2026-03-12T20:57:08Z

src/apm_cli/commands/install.py

                                    prompt_integrator.integrate_package_prompts(
                                        cached_package_info, project_root,
                                        force=force, managed_files=managed_files,
+                                        diagnostics=diagnostics,
                                    )
                                )
                                if prompt_result.files_integrated > 0:


Diagnostics are being threaded through to the prompt/agent/etc. integrators here, but the nearby skill_integrator.integrate_package_skill(...) call in this same integration block is still invoked without diagnostics=diagnostics. That leaves skill name-normalization warnings and sub-skill overwrite notices outside the end-of-install diagnostic summary and may reintroduce inline noise.

This issue also appears on line 1208 of the same file.

Copilot · 2026-03-12T20:57:09Z

src/apm_cli/utils/diagnostics.py

+    @property
+    def has_diagnostics(self) -> bool:
+        """Return True if any diagnostics have been recorded."""
+        return len(self._diagnostics) > 0
+
+    @property
+    def error_count(self) -> int:
+        return sum(1 for d in self._diagnostics if d.category == CATEGORY_ERROR)
+
+    def by_category(self) -> Dict[str, List[Diagnostic]]:
+        """Return diagnostics grouped by category, preserving insertion order."""
+        groups: Dict[str, List[Diagnostic]] = {}
+        for d in self._diagnostics:
+            groups.setdefault(d.category, []).append(d)
+        return groups


DiagnosticCollector claims thread-safety, but query/render paths read self._diagnostics without acquiring the lock (e.g., has_diagnostics, error_count, by_category, and render_summary). This can race with concurrent writers during parallel installs. Consider taking a snapshot of the list under the lock (e.g., with self._lock: diags = list(self._diagnostics)) and base all reads/grouping/rendering on that snapshot, or lock around these reads.

Copilot · 2026-03-12T20:57:09Z

src/apm_cli/utils/diagnostics.py

+    def _render_collision_group(self, items: List[Diagnostic]) -> None:
+        count = len(items)
+        noun = "file" if count == 1 else "files"
+        _rich_warning(
+            f"  ⚠ {count} {noun} skipped — local files exist, not managed by APM"
+        )
+        _rich_info("    Use 'apm install --force' to overwrite")


These renderer messages embed hard-coded status symbols (e.g., "⚠" and "✗"). The codebase already centralizes iconography via STATUS_SYMBOLS and the symbol= parameter on _rich_echo/_rich_warning. To keep output consistent (and avoid future icon changes requiring string edits), consider removing the hard-coded symbols and using _rich_warning(..., symbol='warning') / _rich_echo(..., symbol='cross'|'error') instead.

Copilot AI review requested due to automatic review settings March 12, 2026 20:52

Copilot started reviewing on behalf of danielmeppiel March 12, 2026 20:53 View session

merge: resolve conflict with main in cli-commands.md

a4839b2

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI reviewed Mar 12, 2026

View reviewed changes

danielmeppiel merged commit 35cbad1 into main Mar 12, 2026
9 checks passed

danielmeppiel deleted the feature/diagnostic-collector branch March 12, 2026 21:01

danielmeppiel mentioned this pull request Mar 12, 2026

chore: update Unreleased changelog with PRs merged since v0.7.7 #271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add DiagnosticCollector for structured install diagnostics#267

feat: add DiagnosticCollector for structured install diagnostics#267
danielmeppiel merged 2 commits intomainfrom
feature/diagnostic-collector

danielmeppiel commented Mar 12, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-                diagnostics.warn(
-                    f"Skill name '{raw_skill_name}' normalized to '{skill_name}' ({error_msg})",
-                    package=raw_skill_name,
+                # Use the canonical package identifier for diagnostic grouping,
+                # and keep the raw/normalized skill names in the message.
+                package_name = getattr(
+                    getattr(package_info, "package", None),
+                    "name",
+                    raw_skill_name,
+                )
+                diagnostics.warn(
+                    f"Skill name '{raw_skill_name}' normalized to '{skill_name}' "
+                    f"for package '{package_name}' ({error_msg})",
+                    package=package_name,

Conversation

danielmeppiel commented Mar 12, 2026

Summary

What changed

How it looks

Test results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants