fix(audit): pre-create target dirs in drift replay scratch (#1411)

[sergio-sisternes-epam]

Description

Fix false orphaned drift reports for non-skill primitives on targets with auto_create=False.

During drift replay, the scratch directory is empty, causing integrators with auto_create=False (windsurf, cursor, claude, codex, copilot, gemini, opencode) to skip all non-skill primitives. The diff then reports these correctly-installed files as orphaned.

The fix pre-creates all target root directories in the scratch dir before the replay loop, so the auto_create guard passes.

Fixes #1411

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass (72 drift tests)
• Added tests for new functionality

Test added

tests/unit/install/test_drift_auto_create.py — regression test confirming:

1. Without pre-created target root: auto_create=False skips integration (the bug)
2. With pre-created target root: integration proceeds normally (the fix)

[Copilot]

Pull request overview

Fixes false orphaned drift findings during apm audit by making the drift replay scratch tree more closely mirror a real install for targets where auto_create=False (e.g., windsurf), ensuring non-skill primitives are replayed instead of silently skipped.

Changes:

• Pre-create each resolved target’s root_dir under the drift replay scratch directory before running integrators.
• Add a unit regression test illustrating the auto_create=False guard behavior when the target root directory is missing vs present.

Reviewed changes

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

File	Description
src/apm_cli/install/drift.py	Seeds scratch with target root directories so replay doesn’t skip non-skill primitives for auto_create=False targets.
tests/unit/install/test_drift_auto_create.py	Adds a regression test demonstrating integration skip vs success depending on whether the target root directory exists.

[danielmeppiel]

APM Review Panel: ship_with_followups

Fixes false-orphan drift reports on every auto_create=False AI-coding IDE target, restoring audit truthfulness across the majority IDE matrix.

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

All six active panelists converge: the fix is architecturally correct, security-neutral, and restores a critical user-facing promise -- that apm audit tells the truth about repo cleanliness. The 8-line change seeds scratch-root target directories at exactly the right lifecycle point (after target resolution, before the replay loop), mirroring the real-world invariant that these directories exist on disk when the IDE integration ran. The alternative -- pushing scratch awareness into each integrator -- would have leaked replay concerns into 5+ classes for no user benefit.

The strongest post-merge signal comes from test-coverage-expert: the new unit test is a durable regression trap (the empty-lockfile trick ensures the directory MUST be created by the explicit loop, not as a side-effect). However, the existing integration test test_d2_multi_target_drift_reports_secondary_target_files uses a disjunction (in {'modified','orphaned'}) that accepts the exact false-orphan symptom this PR fixes. That means removing the fix would not break integration CI. This is a real gap on a governed-by-policy surface -- the user promise has no integration-tier assertion that would catch a regression at the symptom level. This follow-up is the single highest-priority item post-merge.

No panelist disagreement exists. The fix is correct, the unit trap is tight, and the follow-ups are all improvement-tier polish or future-proofing.

Aligned with: Governed by policy (audit truthfulness is a governance promise; this fix restores it for the full auto_create=False target set); Multi-harness/multi-host (covers windsurf, cursor, claude, codex, copilot, gemini, opencode -- the majority AI-coding IDE matrix); OSS community-driven (community contributor fix for a community-reported bug; fast triage-to-merge signals contributor respect).

Growth signal. This is a trust-recovery moment worth amplifying: apm audit was lying about repo cleanliness on every popular AI-coding IDE except VSCode. The fix -- from a community contributor -- restores truthfulness across Cursor, Windsurf, Claude Code, Codex, Copilot, Gemini, and OpenCode. Release notes should frame this as "audit now tells the truth on every AI-coding IDE" -- a strong hook for evaluators comparing APM against manual dotfile management.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	2	Architecturally sound: pre-create at the right lifecycle point. Test sequencing could be tighter; duplicated auto_create guard across 5 integrators is a future extraction target.
CLI Logging Expert	0	0	1	Render functions are clean; pre-create loop is silent even under --verbose -- one verbose-only seed log would aid observability at zero default noise.
DevX UX Expert	0	0	1	Fix protects audit-truthfulness across the matrix; tests only exercise windsurf -- parametrization would lock the cross-IDE contract explicitly.
Supply Chain Security Expert	0	0	1	KNOWN_TARGETS are safe static paths and scratch_root is bounded -- no current threat. A path-traversal guard would future-proof against dynamic root_dir overrides.
OSS Growth Hacker	0	0	2	High-value trust-recovery fix from a community contributor on the AI-coding-IDE majority -- worth a thank-you and a release-note beat.
Test Coverage Expert	0	1	1	Unit regression trap is durable (verified by pytest run); integration test_d2 accepts both 'modified' and 'orphaned' so a re-regression to false-orphan would slip past CI.

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

Top 5 follow-ups

1. [Test Coverage Expert] Tighten test_d2 disjunction to assert == 'modified' (or add test_d4_auto_create_false_target_tamper_reports_modified_not_orphaned) -- The integration test currently accepts both 'modified' and 'orphaned', meaning a regression to the exact [BUG] apm audit reports false orphaned drift for non-skill primitives on targets with auto_create=False #1411 symptom would pass CI silently. This is a governed-by-policy surface with no integration-tier guardrail on the fixed behavior.
2. [DevX UX Expert] Parametrize unit test across all 7-8 auto_create=False targets -- Current test exercises only windsurf. A parametrized matrix locks in the cross-IDE contract and catches future targets with divergent root_dir layouts.
3. [Python Architect] Add a side_effect assertion in test_run_replay_precreates_scratch_target_dirs verifying dirs exist on first integrate_package_primitives call -- Current test would still pass if the pre-create were accidentally moved inside the per-dep loop; a sequencing assertion makes the trap tighter.
4. [OSS Growth Hacker] CHANGELOG entry should name affected IDE targets explicitly for ctrl-F discoverability -- Users searching for their specific IDE (cursor, windsurf, claude) will find the fix without reading the full entry.
5. [Python Architect] Extract duplicated auto_create guard from 5 integrators into BaseIntegrator -- Reduces future drift risk as new integrators are added; not blocking because the duplication is stable today.

Architecture

classDiagram
    direction LR
    class TargetProfile {
      <<ValueObject>>
      +name str
      +root_dir str
      +auto_create bool
      +primitives dict
    }
    class BaseIntegrator {
      <<Base>>
      +integrate(target, project_root, ...) IntegrationResult
    }
    class InstructionIntegrator {
      +integrate(target, project_root, ...) IntegrationResult
    }
    class PromptIntegrator {
      +integrate(target, project_root, ...) IntegrationResult
    }
    class AgentIntegrator {
      +integrate(target, project_root, ...) IntegrationResult
    }
    class CommandIntegrator {
      +integrate(target, project_root, ...) IntegrationResult
    }
    class SkillIntegrator {
      +integrate(target, project_root, ...) IntegrationResult
    }
    class run_replay {
      <<IOBoundary>>
      +run_replay(config) Path
    }
    class integrate_package_primitives {
      <<Facade>>
      +integrate_package_primitives(...) dict
    }
    class IntegrationResult {
      <<ValueObject>>
      +files_integrated int
      +files_skipped int
    }
    BaseIntegrator <|-- InstructionIntegrator
    BaseIntegrator <|-- PromptIntegrator
    BaseIntegrator <|-- AgentIntegrator
    BaseIntegrator <|-- CommandIntegrator
    BaseIntegrator <|-- SkillIntegrator
    run_replay ..> integrate_package_primitives : delegates per dep
    integrate_package_primitives ..> InstructionIntegrator : dispatches
    integrate_package_primitives ..> PromptIntegrator : dispatches
    integrate_package_primitives ..> AgentIntegrator : dispatches
    integrate_package_primitives ..> CommandIntegrator : dispatches
    integrate_package_primitives ..> SkillIntegrator : dispatches
    InstructionIntegrator ..> TargetProfile : reads auto_create
    PromptIntegrator ..> TargetProfile : reads auto_create
    AgentIntegrator ..> TargetProfile : reads auto_create
    CommandIntegrator ..> TargetProfile : reads auto_create
    SkillIntegrator ..> TargetProfile : reads auto_create
    InstructionIntegrator ..> IntegrationResult : returns
    class run_replay:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm audit --check drift"] --> B["run_replay(config)"]
    B --> C["_make_scratch_root(project_root)"]
    C --> D["resolve_targets(project_root)"]
    D --> E["_make_integrators()"]
    E --> F["PRE-CREATE LOOP<br/>for target in targets:<br/>  (scratch_root / target.root_dir).mkdir()"]
    F --> G["_ReadOnlyProjectGuard context"]
    G --> H{"for lock_dep in lock.get_all_dependencies()"}
    H --> I["_materialize_install_path"]
    I --> J["integrate_package_primitives<br/>project_root=scratch_root"]
    J --> K{"auto_create check:<br/>(project_root / target.root_dir).is_dir()?"}
    K -- "True (dir pre-created)" --> L["deploy files into scratch"]
    K -- "False (no pre-create)" --> M["return IntegrationResult(0,0,0,[])<br/>FALSE ORPHANED"]
    L --> N["next dep"]
    N --> H
    H -- "all deps done" --> O["diff_scratch_against_project"]
    O --> P["DriftReport"]

    style F fill:#fff3b0,stroke:#d47600
    style M fill:#ffcccc,stroke:#cc0000

Loading

sequenceDiagram
    participant User
    participant CLI as apm audit --check drift
    participant Replay as run_replay
    participant Scratch as scratch tmpdir
    participant Integ as InstructionIntegrator
    participant Diff as diff_scratch_against_project

    User->>CLI: apm audit --check drift
    CLI->>Replay: run_replay(config)
    Replay->>Scratch: mkdir(scratch_root)
    Replay->>Replay: resolve_targets()
    Note over Replay,Scratch: NEW: pre-create target root dirs
    Replay->>Scratch: mkdir(scratch_root / .windsurf)
    Replay->>Scratch: mkdir(scratch_root / .cursor)
    loop each locked dependency
        Replay->>Integ: integrate(target, project_root=scratch_root)
        Integ->>Scratch: check (scratch_root / target.root_dir).is_dir()
        Note over Integ: Previously False -> skip -> false orphan
        Note over Integ: Now True -> integrate correctly
        Integ->>Scratch: write files
    end
    Replay->>Diff: diff_scratch_against_project()
    Diff-->>CLI: DriftReport (clean, no false orphans)
    CLI-->>User: No drift detected

Loading

Recommendation

Merge as-is. The fix is correct, regression-trapped at unit tier, and restores a critical audit-truthfulness promise across the majority IDE matrix. Track the test_d2 disjunction tightening (follow-up #1) as the highest-signal post-merge item -- it is the only path where a future regression could slip past CI undetected on this governed-by-policy surface.

---

Full per-persona findings

Python Architect

• [nit] Test does not defend against pre-create loop being moved inside the per-package block at tests/unit/install/test_drift_auto_create.py
test_run_replay_precreates_scratch_target_dirs asserts the directory exists after run_replay returns, but it would still pass if the pre-create were accidentally moved inside the for-loop (per-dep). Correctness requires the dirs to exist BEFORE the first integrator call.
  Suggested: Add a side_effect to a mock of integrate_package_primitives that asserts (scratch_root / target.root_dir).is_dir() on first call, proving sequencing.
• [nit] Duplicated auto_create guard across 5 integrators is a future extraction candidate at src/apm_cli/integration/instruction_integrator.py:86
instruction_integrator.py:86, prompt_integrator.py:110, agent_integrator.py:111, command_integrator.py:481, skill_integrator.py:341 all duplicate if not target.auto_create and not (project_root / target.root_dir).is_dir(): return .... Not blocking for this PR; a BaseIntegrator._should_skip_target(target, project_root) -> bool would make the guard discoverable and testable in one place.

CLI Logging Expert

• [nit] Pre-create loop is silent even in verbose mode at src/apm_cli/install/drift.py:427
  A single logger.verbose-gated line (e.g. [i] Seeded N scratch target root(s)) would help confirm the fix fired during apm audit --verbose without adding noise for default users.

DevX UX Expert

• [nit] New tests only exercise windsurf, not the full auto_create=False target matrix at tests/unit/install/test_drift_auto_create.py:21
pytest.mark.parametrize across the 7-8 auto_create=False targets would prove the contract holds across the matrix and catch a future target whose root_dir layout diverges. Low risk today since the fix loop is target-agnostic, but parametrization makes the regression net explicit.

Supply Chain Security Expert

• [nit] Future-proof against dynamic _target.root_dir path-traversal at src/apm_cli/install/drift.py:431
  All KNOWN_TARGETS root_dir values are safe static strings today, and scratch_root is bound outside the project tree. If a future target profile or apm.yml override introduces a dynamic root_dir, the join would silently escape scratch. A one-line assertion that the resolved path stays under scratch_root would future-proof.

OSS Growth Hacker

• [nit] CHANGELOG entry should name affected IDE targets explicitly at CHANGELOG.md
  Users searching their IDE name will ctrl-F the changelog. Listing windsurf, cursor, claude, codex, copilot, gemini, opencode beats the generic "non-VSCode targets" phrasing for discoverability.
• [nit] Inline comment in drift.py could name affected targets at src/apm_cli/install/drift.py:427
  A short comment listing the auto_create=False targets makes the blast radius obvious to future readers without cross-referencing targets.py.

Test Coverage Expert

• [recommended] Integration test accepts false-orphan kind -- cannot catch [BUG] apm audit reports false orphaned drift for non-skill primitives on targets with auto_create=False #1411 re-regression at tests/integration/test_drift_check.py
test_d2_multi_target_drift_reports_secondary_target_files asserts kinds['.claude/rules/rules.md'] in {'modified','orphaned'}. Before this fix the audit falsely reported orphaned; after it reports modified. The disjunction accepts both, so removing the pre-create loop would NOT fail this test. The user-visible promise has no integration-tier assertion that would fail on the exact symptom of [BUG] apm audit reports false orphaned drift for non-skill primitives on targets with auto_create=False #1411.
  Suggested: Either tighten test_d2 to assert == 'modified' or add a sibling integration test (e.g. test_d4_auto_create_false_target_tamper_reports_modified_not_orphaned) that pins the fix at integration tier.
  Proof (missing at): tests/integration/test_drift_check.py::test_d4_auto_create_false_target_tamper_reports_modified_not_orphaned -- proves: apm audit reports 'modified' (not false-orphan) when a deployed file is tampered on an auto_create=False target after a clean install [devx,governed-by-policy]
  assert kinds['.claude/rules/rules.md'] == 'modified'
• [nit] Unit regression trap is durable (verified by running tests) at tests/unit/install/test_drift_auto_create.py
test_run_replay_precreates_scratch_target_dirs uses an empty lockfile so the pre-create loop body never runs as a side-effect of dep integration -- the asserted directory MUST be created by the explicit loop. This catches the pre-create-loop being removed. test_auto_create_false_targets_still_integrate_in_scratch directly confirms the InstructionIntegrator contract.
  Proof (passed): tests/unit/install/test_drift_auto_create.py::test_run_replay_precreates_scratch_target_dirs -- proves: run_replay pre-creates target root dirs in scratch before the integration loop [devx]
  assert (scratch / windsurf.root_dir).is_dir()

Auth Expert -- inactive

Diff touches only drift.py + a new test file; no auth.py, token_manager.py, azure_cli.py, github_downloader.py, marketplace/client.py, github_host.py, install/validation.py, install/pipeline.py, deps/registry_proxy.py -- and no host classification, token-source, or clone-URL change.

Doc Writer -- inactive

No README/CHANGELOG/MANIFESTO/docs/.apm/.github prose changed; the diff is a runtime bug fix + unit test, not a documentation surface. (growth-hacker still flags an OPTIONAL CHANGELOG-entry call-out for release time -- not a doc drift today.)

Performance Expert -- inactive

Diff is outside the perf hot path (cache/, deps/, install/phases/, install/pipeline.py, install/resolve.py, scripts/perf/) and makes no perf claim; pre-creating up to ~10 directories per audit run is negligible vs. the existing replay cost.

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

[danielmeppiel]

Folded recommended follow-ups -- commit 87eefac

Per panel recommendation, folded the single RECOMMENDED follow-up plus one related nit directly into this PR instead of deferring to a tracking issue.

Items folded

• [Test Coverage Expert -- recommended] Tighten test_d2 disjunction at integration tier.
  • tests/integration/test_drift_check.py:558-590 -- assert kinds[".claude/rules/rules.md"] == "modified" (was in {"modified","orphaned"}).
  • tests/integration/test_drift_check.py:537-547 -- updated TestSectionDMultiTarget docstring to describe the post-[BUG] apm audit reports false orphaned drift for non-skill primitives on targets with auto_create=False #1411 invariant and list the affected auto_create=False IDE matrix.
  • Mutation-break verified: temporarily reverting the pre-create loop in src/apm_cli/install/drift.py:431-433 turns this assertion RED with {'.claude/rules/rules.md': 'orphaned'} -- the exact [BUG] apm audit reports false orphaned drift for non-skill primitives on targets with auto_create=False #1411 symptom. Production code restored byte-for-byte before commit.
• [OSS Growth Hacker -- nit] CHANGELOG entry naming each affected IDE.
  • CHANGELOG.md:10-12 -- new ### Fixed block under ## [Unreleased] lists cursor, windsurf, claude, codex, copilot, gemini, opencode for ctrl-F discoverability.

Items deferred

• [DevX UX -- nit] Parametrize unit test across all auto_create=False targets -- separable; would expand the unit harness and is not on this PR's critical path.
• [Python Architect -- nit] Add side_effect sequencing assertion in test_run_replay_precreates_scratch_target_dirs -- separable; current empty-lockfile trap already catches the most likely regression mode (loop removal).
• [Python Architect -- nit] Extract duplicated auto_create guard into BaseIntegrator._should_skip_target -- cross-cutting refactor across 5 integrators; separate PR.
• [CLI Logging -- nit] Verbose-only seed-count log line -- separable observability improvement.
• [Supply Chain Security -- nit] Path-traversal guard for dynamic root_dir overrides -- future-proofing; no current threat (all KNOWN_TARGETS root_dir values are safe static strings).

Local validation

• uv run --extra dev ruff check src/ tests/ -- silent.
• uv run --extra dev ruff format --check src/ tests/ -- 1034 files already formatted.
• uv run --extra dev pytest tests/integration/test_drift_check.py tests/unit/install/test_drift_auto_create.py -- 35 passed.
• Mutation-break gate on the tightened test_d2: RED on production guard removal, GREEN with guard restored.

CI status

Workflow runs for SHA 87eefac were not auto-created on push -- this repo gates outside-contributor PR runs behind maintainer approval. @danielmeppiel please click "Approve and run" on the Actions tab to spin up CI for the new SHA. All local checks passed.

Verdict

ready-to-merge once CI runs are approved and green.

[danielmeppiel]

Rebased onto current main and resolved conflicts at 36d878f. Local validation: lint silent, custom CI guard (no str-relative-to pattern) silent in src/ and no new occurrences in tests/ diff, targeted tests pass (1339 passed), mutation-break gate verified (removing the pre-create-dirs guard fails 2 regression tests including test_d2).