fix(install): exclude self-entry from dry-run orphan preview

[mia106dev]

TL;DR

apm install --dry-run falsely lists the project's own includes: auto
self-managed files under "Files that would be removed (packages no longer in
apm.yml)". The real install never removes them — this is a dry-run preview
bug only. The fix makes the orphan preview skip the synthesized lockfile
self-entry, so the preview matches actual install behaviour.

Problem

For a project using includes: auto, LockFile.from_yaml synthesizes a
virtual dependency keyed by _SELF_KEY (".") from the flat
local_deployed_files field, to unify traversal across real dependencies and
the project's own local content (src/apm_cli/deps/lockfile.py, from_yaml).

detect_orphans (src/apm_cli/drift.py) walks every entry in
existing_lockfile.dependencies and flags any key not present in the
manifest-derived intended_dep_keys as orphaned:

for dep_key, dep in existing_lockfile.dependencies.items():
    if dep_key not in intended_dep_keys:
        orphaned.update(dep.deployed_files)

The synthesized _SELF_KEY (".") can never appear in intended_dep_keys
(it is not a manifest dependency), so on a full install every self-include was
reported as an orphan to be removed.

This is preview-only over-reporting. The real paths are already correct and
were never affected:

• Replay guards the self-entry to re-deploy it, not remove it —
  src/apm_cli/install/drift.py:481,495 (if lock_dep.local_path == _SELF_KEY).
• The cleanup phase independently skips it —
  src/apm_cli/install/phases/cleanup.py:79 (if _orphan_key == _SELF_KEY).

detect_orphans' only non-test caller is the dry-run preview renderer
(src/apm_cli/install/presentation/dry_run.py:70), which confirms the blast
radius is the preview text alone.

Approach

Skip _SELF_KEY inside detect_orphans. Orphan-ness is a property the
function itself owns, so the guard belongs here — next to the sibling guards
already present in the real cleanup/replay paths — rather than filtered at the
single caller. Synthesis stays in from_yaml because other consumers rely on
the self-entry existing.

Implementation

+    from apm_cli.deps.lockfile import _SELF_KEY
+
     orphaned: builtins.set = builtins.set()
     if only_packages or not existing_lockfile:
         return orphaned
     for dep_key, dep in existing_lockfile.dependencies.items():
+        # The synthesized self-entry (key ".") holds the project's own
+        # includes:auto content, which the real install always re-deploys
+        # (never removes). It can never appear in the manifest-derived
+        # intended set, so skip it to avoid over-counting self-includes as
+        # removals in the dry-run preview.
+        if dep_key == _SELF_KEY:
+            continue
         if dep_key not in intended_dep_keys:
             orphaned.update(dep.deployed_files)
     return orphaned

The _SELF_KEY import is function-scoped, matching the codebase's established
lazy-import idiom for the drift ↔ lockfile cycle.

Validation

• New unit tests in tests/unit/test_drift_detection.py:
  • test_self_entry_never_orphaned — a stub lockfile with a self-entry plus a
    genuinely dropped package: only the dropped package's files are returned.
  • test_self_include_survives_real_from_yaml_roundtrip — exercises the
    real LockFile.from_yaml synthesis → detect_orphans path, so a future
    change to either the self-key value or the synthesis shape that reintroduces
    the false alarm fails here.
• Red→green confirmed: test_self_entry_never_orphaned fails on main
  (self_a.md/self_b.md wrongly returned), passes with the fix.
• End-to-end check against the real parse path: with the manifest emptied, only
  a genuinely dropped package's file is reported; self-includes
  (.claude/skills/...) no longer appear.
• Blast-radius unit suites green (test_drift_detection, test_install_update,
  tests/unit/install/): 1831 passed.
• Lint clean: ruff check + ruff format --check on the changed files.

How to test

uv run pytest tests/unit/test_drift_detection.py -q

Or reproduce the original symptom: in a project that uses includes: auto with
a committed apm.lock.yaml, run apm install --dry-run — before this change
the project's own deployed files are listed under "Files that would be
removed"; after, they are not.

Scope notes

• src/apm_cli/drift.py is not under the OpenAPM Mode-B critical-path allowlist
  (tests/spec_conformance/critical_paths.txt), so no spec anchor/manifest/
  marker or apm-spec-waiver: is required.
• Out of scope (noted by review): the dep_key == _SELF_KEY idiom now appears
  in several consumers; a LockFile.real_dependencies() accessor to DRY it is a
  separate refactor.

[Copilot]

Pull request overview

Fixes a dry-run-only drift preview bug where apm install --dry-run incorrectly reported the project’s own includes: auto self-managed files as “orphans” (removals), by excluding the synthesized lockfile self-entry (_SELF_KEY, ".") from orphan detection.

Changes:

• Update detect_orphans() to skip the synthesized self dependency key (".") so dry-run orphan previews match real install behavior.
• Add unit tests covering both a stubbed lockfile case and a real LockFile.from_yaml() synthesis roundtrip.
• Add a changelog entry for the fix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/apm_cli/drift.py	Skips _SELF_KEY during orphan detection to avoid false removals in dry-run preview.
tests/unit/test_drift_detection.py	Adds regression tests ensuring the self-entry is never treated as orphaned, including a real from_yaml parse path.
CHANGELOG.md	Documents the dry-run orphan preview fix under Unreleased/Fixed.

[sergio-sisternes-epam]

APM Review Panel: ship_with_followups

Clean trust-repair fix for dry-run orphan false-positive; ship now, fold CHANGELOG nits, defer accessor refactor.

cc @mia106dev @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

All six active panelists converge: the fix is correct, minimal, and well-tested. No specialist disagreements to resolve. The python-architect's real_dependencies() accessor recommendation is the only architecturally substantive finding, and the PR author already called it out as deferred scope -- this is the right call for a community contributor's bugfix PR; we do not pile refactors onto external contributions.

The doc-writer caught two nits on the CHANGELOG entry (attribution format and em-dash encoding) that should be folded before merge -- they're mechanical, break established conventions, and take 30 seconds to fix. The test-coverage-expert validated that the new from_yaml roundtrip test is the right shape for a regression trap (9 passed, 0.21s, integration-with-fixtures tier). The missing e2e variant requires GITHUB_APM_PAT and is correctly scoped as a follow-up, not a gate.

Strategically, this is a low-risk, high-trust fix. Dry-run fidelity is load-bearing for adoption: users who see false warnings in preview mode lose confidence in the tool before they ever run a real install. Shipping this quickly signals that we treat preview accuracy as a first-class contract.

Aligned with: Pragmatic as npm -- Dry-run telling the truth is table-stakes for any package manager; this fix closes a gap that npm/pip users would never tolerate. OSS community-driven -- Community contributor fix, cleanly scoped, no refactor burden imposed. Exactly the kind of PR that should merge fast to keep the contributor loop tight.

Growth signal. The oss-growth-hacker's note is worth amplifying at release time: '--dry-run now tells the truth' is a concrete trust signal. Every false positive in a preview surface has outsized negative adoption impact because it poisons the user's mental model before real usage begins. Consider tracking dry-run accuracy as a quality metric in release notes -- it reinforces APM's 'predictable by default' positioning against tools where preview modes are an afterthought.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	1	Fix is correct and minimal; the scattered _SELF_KEY guard (now 10+ sites) is the real architectural debt worth a follow-up accessor.
CLI Logging Expert	0	0	0	Fix correctly silences false orphan noise in dry-run; output parity with real install restored, no output contract violations.
DevX UX Expert	0	0	1	Fix correctly restores dry-run fidelity for includes:auto projects; CHANGELOG entry is clear; no UX regressions.
Supply Chain Security Expert	0	0	0	Preview-only orphan guard; no file-deletion, integrity, or provenance paths affected. No security regression.
OSS Growth Hacker	0	0	1	Good trust-repair fix for dry-run; CHANGELOG entry is user-facing and clear -- minor story-angle opportunity in release notes.
Doc Writer	0	0	3	CHANGELOG entry is accurate and scoped correctly; two nits on attribution format and em-dash encoding; no doc-site pages need updating.
Test Coverage Expert	0	2	1	Two new unit tests defend the _SELF_KEY guard well; from_yaml roundtrip test is near-integration tier. No e2e gap blocks ship.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Doc Writer] Fix CHANGELOG attribution format to match established (by @author, #NNNN) pattern and drop the em-dash. -- Consistency with every other CHANGELOG entry; ASCII-only encoding rule.
2. [Python Architect] Add LockFile.real_dependencies accessor to centralize the _SELF_KEY guard across 10+ call sites. -- Eliminates the entire class of 'forgot to skip self-entry' bugs; each new consumer today must independently remember the guard.
3. [Test Coverage Expert] Add an includes:auto variant to the e2e dry-run integration test. -- Certifies the full CLI path end-to-end; currently the e2e fixture does not exercise self-entry synthesis so this class of regression would not be caught at the integration tier.
4. [Test Coverage Expert] Add parametrize case for lockfile with only a self-entry and zero real deps. -- Trivial edge case that makes the empty-set return explicit; one line of test code.
5. [DevX UX Expert] Document dry-run fidelity contract in CLI reference or --help text. -- The promise that --dry-run output exactly matches real install behavior is enforced by code but never stated to users.

Architecture

classDiagram
    direction LR
    class LockFile {
      ``Dataclass``
      +dependencies: dict~str, LockedDependency~
      +from_yaml(text) LockFile
      +to_yaml() str
    }
    class LockedDependency {
      ``Dataclass``
      +repo_url: str
      +deployed_files: list~str~
      +local_path: str
    }
    class detect_orphans {
      ``Function``
      +detect_orphans(lockfile, intended, only_packages) set
    }
    class DryRunPresenter {
      ``Module: dry_run.py``
      +present_dry_run_preview()
    }
    class CleanupPhase {
      ``Module: cleanup.py``
      +run_orphan_cleanup()
    }
    class InstallDrift {
      ``Module: install/drift.py``
      +replay_drift()
    }
    LockFile *-- LockedDependency : contains
    DryRunPresenter ..> detect_orphans : calls
    detect_orphans ..> LockFile : reads dependencies
    CleanupPhase ..> LockFile : iterates dependencies
    InstallDrift ..> LockFile : iterates dependencies
    note for LockFile "_SELF_KEY guard duplicated in every consumer -- 10+ sites"
    class detect_orphans:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm.lock.yaml on disk"] -->|LockFile.from_yaml| B["LockFile.dependencies\ndicts incl. _SELF_KEY='.'"]
    B --> C{"dry-run mode?"}
    C -->|Yes| D["dry_run.py:70\npresent_dry_run_preview()"]
    D -->|calls| E["detect_orphans()"]
    E -->|NEW: skip _SELF_KEY| F["Return orphaned file set\n(self-includes excluded)"]
    F --> G["logger.progress()\npreview removed files"]
    C -->|No: real install| H["cleanup.py:74\nrun_orphan_cleanup()"]
    H -->|existing guard: skip _SELF_KEY| I["remove_stale_deployed_files()"]
    style E fill:#fff3b0,stroke:#d47600
    style F fill:#fff3b0,stroke:#d47600

Loading

sequenceDiagram
    participant User
    participant CLI as apm install --dry-run
    participant DryRun as dry_run.py
    participant Detect as drift.detect_orphans
    participant LF as LockFile
    User->>CLI: apm install --dry-run
    CLI->>DryRun: present_dry_run_preview()
    DryRun->>LF: from_yaml(apm.lock.yaml)
    LF-->>DryRun: LockFile with synthesized self-entry
    DryRun->>Detect: detect_orphans(lockfile, intended_keys)
    loop each dep_key in lockfile.dependencies
        alt dep_key == _SELF_KEY
            Detect-->>Detect: skip (NEW guard)
        else dep_key not in intended
            Detect-->>Detect: add to orphaned set
        end
    end
    Detect-->>DryRun: orphaned files (no self-includes)
    DryRun-->>User: preview: N files would be removed

Loading

Recommendation

Ship after folding the two CHANGELOG nits (attribution format + em-dash). The fix is correct, the tests are solid including a real from_yaml roundtrip regression trap, no security or UX regressions, and every panelist agrees. The accessor refactor and e2e gap are legitimate follow-ups but gating a community contributor's clean two-line fix on them would be exactly the kind of scope creep that loses contributors. Merge fast, file the follow-ups, and credit @mia106dev in the release notes.

---

Full per-persona findings

Python Architect

• [recommended] LockFile should expose a real_dependencies() accessor to centralize the _SELF_KEY guard at src/apm_cli/deps/lockfile.py
  _SELF_KEY is imported and guarded against in 10+ call sites across 7 files. Each consumer independently does if key == _SELF_KEY: continue. A LockFile.real_dependencies property would eliminate the entire class of bugs this PR fixes.
  Suggested: @property def real_dependencies(self) -> dict[str, LockedDependency]: return {k: v for k, v in self.dependencies.items() if k != _SELF_KEY}

• [nit] detect_orphans docstring should mention self-entry exclusion at src/apm_cli/drift.py:200
  The Returns section should note that the synthesized self-entry (key ".") is always excluded from the returned set.

CLI Logging Expert

No findings.

DevX UX Expert

• [nit] The dry-run fidelity contract (that --dry-run output exactly matches real install) is enforced by code but never stated in CLI reference docs or --help text.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

• [nit] CHANGELOG entry misses the emotional hook. Suggested rephrasing: "Fixed a false-positive in apm install --dry-run that incorrectly warned includes: auto projects their own files would be removed -- the real install was never affected, but the preview was misleading."

Auth Expert -- inactive

PR touches only src/apm_cli/drift.py, tests/unit/test_drift_detection.py, and CHANGELOG.md -- none of which involve token management, credential resolution, or remote-host authentication.

Doc Writer

• [nit] Attribution format -- by @mia106dev (#2069) breaks the established (by @author, #NNNN) pattern. Change to (by @mia106dev, #2069).

• [nit] The separator before attribution is a Unicode em-dash (U+2014) which violates the ASCII-only encoding rule. Switching to parenthesised attribution resolves this.

• [nit] CHANGELOG sentence is slightly overloaded with internal mechanism detail ("synthesized lockfile self-entry"). Optional: trim to user-visible behaviour only.

Test Coverage Expert

• [nit] Missing edge case: lockfile with ONLY a self-entry (no other deps) should return empty set. One parametrize entry would make this explicit.
  Proof (missing): tests/unit/test_drift_detection.py::test_self_entry_only_lockfile_returns_empty -- proves: A project with includes:auto but zero deps does not report self-includes as orphans

• [recommended] test_self_include_survives_real_from_yaml_roundtrip uses real LockFile.from_yaml() -- effectively an integration-with-fixtures test. RIGHT shape for a regression trap.
  Proof (passed, integration-with-fixtures): tests/unit/test_drift_detection.py::test_self_include_survives_real_from_yaml_roundtrip -- proves: LockFile.from_yaml synthesis + detect_orphans guard agree on _SELF_KEY value. Run: 9 passed, 0.21s.
  self.assertEqual(result, {".apm/x.md"})

• [recommended] E2E dry-run test (tests/integration/test_install_dry_run_e2e.py) does not use includes:auto in its fixture. Adding a variant would certify the full CLI path; requires GITHUB_APM_PAT so this is a follow-up.
  Proof (missing, e2e): tests/integration/test_install_dry_run_e2e.py::test_install_dry_run_includes_auto_no_false_orphans

Performance Expert -- inactive

The change adds a trivial guard in detect_orphans() which runs only during --dry-run preview -- not in any hot path.

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}

[sergio-sisternes-epam]

@mia106dev thanks for the clean fix! One pre-merge ask before we queue this:

The CHANGELOG entry uses a Unicode em-dash (U+2014) and a non-standard attribution format. Please change line 15 from:

  which never removes them. — by @mia106dev (#2069)

to:

  which never removes them. (by @mia106dev, #2069)

This matches the established pattern in every other CHANGELOG entry and satisfies the ASCII-only encoding rule. Everything else looks great -- once that's pushed the PR is ready to merge.