-
Notifications
You must be signed in to change notification settings - Fork 45
feat: add DiagnosticCollector for structured install diagnostics #267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,14 @@ | ||
| """Base integrator with shared collision detection and sync logic.""" | ||
|
|
||
| import re | ||
| import sys | ||
| from pathlib import Path | ||
| from typing import Dict, List, Optional, Set | ||
|
|
||
| from dataclasses import dataclass, field | ||
|
|
||
| from apm_cli.compilation.link_resolver import UnifiedLinkResolver | ||
| from apm_cli.primitives.discovery import discover_primitives | ||
| from apm_cli.utils.console import _rich_warning | ||
|
|
||
|
|
||
| @dataclass | ||
|
|
@@ -51,6 +51,7 @@ def check_collision( | |
| rel_path: str, | ||
| managed_files: Optional[Set[str]], | ||
| force: bool, | ||
| diagnostics=None, | ||
| ) -> bool: | ||
| """Return True if *target_path* is a user-authored collision. | ||
|
|
||
|
|
@@ -60,7 +61,8 @@ def check_collision( | |
| 3. ``rel_path`` is **not** in the managed set (→ user-authored) | ||
| 4. ``force`` is ``False`` | ||
|
|
||
| When a collision is detected a warning is emitted to *stderr*. | ||
| When *diagnostics* is provided the skip is recorded there; | ||
| otherwise a warning is emitted via ``_rich_warning``. | ||
|
|
||
| .. note:: Callers must pre-normalize *managed_files* with | ||
| forward-slash separators (see ``normalize_managed_files``). | ||
|
|
@@ -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." | ||
| ) | ||
|
Comment on lines
64
to
+86
|
||
| return True | ||
|
|
||
| @staticmethod | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withoutdiagnostics=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.