fix: apm install no longer adopts files not produced by installed plugins (closes #1199)

[sergio-sisternes-epam]

Fixes #1199

Problem

apm install integrates files in .github/instructions/ that pre-exist but are not produced by any installed plugin. This causes the unmanaged-files audit to report clean when it should flag them.

Approach

During integration, check file provenance against the declared plugin manifests. Only files produced by installed plugins should be treated as managed; pre-existing files without provenance should be left untouched.

---

Draft PR -- implementation in progress.

[Copilot]

Pull request overview

Adds unit-level regression coverage around the unmanaged-files policy check for governance directories, and updates the changelog to mention the #1199 fix intent.

Changes:

• Add new unit tests asserting hand-rolled files under .github/instructions/, .github/hooks/, and .github/agents/ are reported as unmanaged unless present in lockfile provenance (including local_deployed_files).
• Add a unit test asserting a managed directory-prefix in one governance directory (e.g. .github/skills/.../) does not mask unmanaged files in another (e.g. .github/instructions/).
• Add a CHANGELOG.md entry under Fixed referencing the #1199 regression area.

Reviewed changes

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

File	Description
tests/unit/policy/test_policy_checks.py	Adds regression tests for unmanaged-files auditing across multiple governance directories and lockfile provenance sources.
CHANGELOG.md	Adds an Unreleased “Fixed” entry referencing the unmanaged-files hardening / #1199 area.

[github-actions]

APM Review Panel: needs_rework

PR #1256 adds five valuable regression-trap tests for #1199 but ships without the src/ install-pipeline fix that caused the bug -- the audit was already correct before this PR.

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

The panel converges on a clear structural diagnosis: the five new tests in tests/unit/policy/test_policy_checks.py correctly lock in the behavior of _check_unmanaged_files -- a pure function that was already correct before this PR. The actual defect described in #1199 lives upstream in BaseIntegrator.check_collision (returns False when managed_files is None) and in LockfilePhase._attach_deployed_files, which together allow pre-existing, unmanaged governance files to be absorbed into deployed_files during apm install. No src/ file is touched by this PR. The PR body acknowledges implementation is in progress, yet the PR is marked non-draft and the CHANGELOG entry frames it as a completed fix -- that mismatch is the primary concern.

The supply-chain-security finding on the scan-cap fail-open path (test_rglob_cap_skips_check asserts result.passed=True when file count exceeds _MAX_UNMANAGED_SCAN_FILES) is the second-highest signal finding in this panel. A governance-directory adversary with file-write access can silence the entire unmanaged-files audit by creating N+1 files, and this PR regression-pins that bypass as expected behavior -- raising the cost of ever making the cap fail-closed. This is a secure-by-default / governed-by-policy surface and the pinning warrants an explicit follow-up issue before or concurrent with the src/ fix.

The CHANGELOG entry and doc-writer findings are straightforward: the entry describes test evidence rather than user-observable behavior change, cites the PR number instead of the closed issue, and will confuse users reading the release notes. The oss-growth-hacker's suggested reframe is strictly better user language and should be adopted with no loss of accuracy. The DevX / CLI-logging finding (bare paths, no remediation hint) is pre-existing and low-urgency but worth a follow-up issue so it does not get regression-pinned in turn.

Dissent. The test-coverage-expert returned outcome: unknown (pytest unavailable in runner) rather than a confirmed pass/fail. Per panel weighting rules, unknown carries no evidential weight; the five tests are assessed by code-read only and treated as opinion-tier for arbitration purposes. This does not change the stance -- the structural absence of a src/ fix is independently verified by python-architect's code trace, which is not execution-dependent.

Aligned with: Secure by default (partial regression: scan-cap fail-open path is now regression-pinned as passing, which is the wrong default for a security audit surface; needs a follow-up to convert to warn or block). Governed by policy (positive: five regression tests lock in correct audit behavior across five scenarios; the install-pipeline fix that would make the governance promise end-to-end correct is still outstanding). OSS community driven (external contributor is engaging with a genuine bug; follow-ups should be framed as a clear next-step invitation).

Growth signal. The unmanaged-files audit is a 'Governed by policy' trust anchor, and silent drift (install adopting files it did not produce) is the failure mode that erodes confidence in regulated or multi-team environments. Once the src/ fix lands, the release note should lead with 'APM's audit tells you the truth -- even when the truth is inconvenient.' Worth a FAQ entry and a 'trust and correctness' beat in the next release post.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	1	Regression tests are structurally sound and exercise real code paths, but the install-path fix that caused #1199 is absent -- the audit was already correct; the bug is in check_collision / LockfilePhase.
CLI Logging Expert	0	0	1	Test detail assertions match the implementation output contract; no CLI output regressions introduced. One pre-existing nit: detail lines omit an actionable hint.
DevX UX Expert	0	1	1	Tests protect the audit correctness contract; user-facing error message names files but gives no remediation hint. Minor gap worth a follow-up.
Supply Chain Security Expert	0	2	1	Tests-only PR; implementation already correct. Two concerns: fail-open scan-cap is now regression-pinned as intended behavior, and the security-critical audit surface lacks integration-with-fixtures coverage.
OSS Growth Hacker	0	0	1	Trust fix with a solid story angle; CHANGELOG entry slightly undersells the user promise -- one word swap would make it shareable.
Doc Writer	0	1	2	CHANGELOG entry misattributes the fix as a test addition and cites the PR number instead of the closed issue (#1199); recommend rewording to describe the behavioral fix.
Test Coverage Expert	0	0	1	Five well-targeted regression-trap unit tests for #1199; unit tier is adequate for this pure-function surface; no blocking gaps. S7 probe skipped (pytest unavailable in runner).

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

Top 5 follow-ups

1. [Python Architect] Add the src/ install-pipeline fix: guard BaseIntegrator.check_collision when managed_files is None, and/or intersect LockfilePhase._attach_deployed_files against ctx.written_files before committing deployed_files. -- The root cause of [bug] apm install integrates files in .github/instructions/ that are not produced by an installed plugin #1199 -- apm install silently adopting pre-existing unmanaged governance files -- is unresolved. The CHANGELOG entry and PR title claim the issue is closed; it is not.
2. [Supply Chain Security Expert] Open a follow-up issue to change the scan-cap path from fail-open (result.passed=True) to fail-warn or fail-closed before this regression pin is merged. -- Adversarial file creation can silence the unmanaged-files audit entirely by exceeding _MAX_UNMANAGED_SCAN_FILES. Pinning the current behavior as expected makes the cap harder to fix later; this is a governed-by-policy / secure-by-default surface.
3. [Supply Chain Security Expert] Add an integration-with-fixtures test: run apm install against a temp repo containing a hand-rolled .github/hooks/ file not produced by any plugin and assert non-zero exit. -- .github/hooks/ is a code-execution surface; unit-only coverage does not verify the audit fires at the CLI boundary where the bug manifests.
4. [Doc Writer] Rewrite the CHANGELOG entry to describe the behavioral fix (not the tests), cite closes #1199 instead of #1256, and use user-facing language per the oss-growth-hacker suggestion. -- The Fixed section must answer 'what changed for users'; the current entry describes test evidence and will confuse readers scanning release notes.
5. [DevX UX Expert] Append a remediation hint to each unmanaged-file detail line in the CheckResult output (pre-existing gap, now regression-pinned by new tests). -- Package-manager UX contract: an actionable diagnostic names the next command. Users seeing a bare path list with no suggested fix will open support issues instead of self-resolving.

Architecture

classDiagram
    direction LR

    class UnmanagedFilesPolicy {
      <<ValueObject>>
      +action str
      +directories list[str]
    }

    class LockFile {
      <<Repository>>
      +dependencies dict[str, LockedDependency]
      +local_deployed_files list[str]
      +read(path) LockFile
      +write(path) None
    }

    class LockedDependency {
      <<ValueObject>>
      +deployed_files list[str]
      +deployed_file_hashes dict
    }

    class _check_unmanaged_files {
      <<Pure>>
      +call(project_root, lock, policy) CheckResult
    }

    class CheckResult {
      <<ValueObject>>
      +name str
      +passed bool
      +message str
      +details list[str]
    }

    class BaseIntegrator {
      <<AbstractBase>>
      +check_collision(target_path, rel_path, managed_files, force) bool
      +validate_deploy_path(rel_path) bool
    }

    class InstructionIntegrator {
      <<ConcreteIntegrator>>
      +integrate_instructions_for_target(target, pkg_info, project_root) IntegrationResult
    }

    class LockfilePhase {
      <<CommandPhase>>
      +_attach_deployed_files(lockfile) None
    }

    class TestUnmanagedFilesPolicy {
      <<TestClass>>
      +test_handrolled_instruction_flagged_as_unmanaged()
      +test_handrolled_file_not_masked_by_deployed_deps()
      +test_handrolled_file_across_governance_dirs()
      +test_local_deployed_files_not_flagged()
      +test_dir_prefix_does_not_mask_instructions()
    }

    LockFile *-- LockedDependency : contains
    _check_unmanaged_files ..> LockFile : reads deployed_files
    _check_unmanaged_files ..> UnmanagedFilesPolicy : reads config
    _check_unmanaged_files ..> CheckResult : returns
    InstructionIntegrator --|> BaseIntegrator
    LockfilePhase ..> InstructionIntegrator : calls integrate
    LockfilePhase ..> LockFile : writes deployed_files
    TestUnmanagedFilesPolicy ..> _check_unmanaged_files : calls directly
    TestUnmanagedFilesPolicy ..> LockFile : constructs fixture

    class TestUnmanagedFilesPolicy:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A(["apm install CLI entry"])
    B["integrate_instructions_for_target\ninstruction_integrator.py:60"]
    C{"BaseIntegrator.check_collision\nbase_integrator.py:61\nmanaged_files is None?"}
    D["returns False\nBUG: pre-existing file not skipped"]
    E["Copy file to governance dir\n.github/instructions/*.md"]
    F["Append to deployed_files"]
    G["LockfilePhase._attach_deployed_files\ninstall/phases/lockfile.py:126\nwrites deployed_files to lockfile"]
    H["apm.lock.yaml written\npre-existing file now appears managed"]
    I(["apm audit / policy check"])
    J["_check_unmanaged_files\npolicy_checks.py:685\nrglob governance dirs"]
    K{"rel in deployed set?"}
    L["File appears managed\nWRONG: was hand-rolled"]
    M["File flagged as unmanaged\nCORRECT"]
    N["TestUnmanagedFilesPolicy\n5 new regression tests\ncalls _check_unmanaged_files directly\nwith hand-crafted LockFile fixtures"]
    O["Tests PASS: audit function correct\nBug path in install NOT covered"]

    A --> B
    B --> C
    C -->|"managed_files is None"| D
    D --> E
    E --> F
    F --> G
    G --> H
    H --> I
    I --> J
    J --> K
    K -->|"yes - pre-existing file was added"| L
    K -->|"no"| M
    N -.->|"exercises"| J
    N -.->|"result"| O

Loading

Recommendation

Hold for the src/ install-pipeline fix. The five regression tests are a net positive and should be preserved, but merging this PR as-is would close #1199 in the tracker while the root cause remains live in production -- a silent breaking promise to users and a trust risk for the 'Governed by policy' differentiator. The highest-leverage next action for the author is to add the BaseIntegrator.check_collision / LockfilePhase._attach_deployed_files guard (python-architect's diagnosis), update the CHANGELOG entry to user-facing language citing closes #1199, and open a separate issue for the scan-cap fail-open path before or concurrent with merge. The test additions can land as-is once those three items are addressed.

---

Full per-persona findings

Python Architect

• [recommended] Install-path provenance fix is missing; tests exercise the already-correct audit, not the defective code path at src/apm_cli/integration/base_integrator.py:82
  The PR body states 'check file provenance against the declared plugin manifests' as the approach. Tracing the actual bug: BaseIntegrator.check_collision returns False when managed_files is None, so pre-existing files in governance dirs ARE deployed and appear in dep.deployed_files, making _check_unmanaged_files report them as managed. The five new tests call _check_unmanaged_files directly with hand-crafted LockFile fixtures -- they prove the audit function is correct (which it was before this PR) but do not cover the install pipeline path (integrate_instructions_for_target -> LockfilePhase._attach_deployed_files) where the provenance leak occurs.
  Suggested: The provenance fix belongs in integrate_instructions_for_target: before appending target_path to deployed_files, assert the file was written by this install run. Alternatively, teach LockfilePhase._attach_deployed_files to intersect dep_files against ctx.written_files rather than all files returned by the integrator. Tests for the fix belong in tests/unit/install/ or tests/integration/.
  Proof (missing at integration-with-fixtures): tests/unit/install/ -- proves: apm install does not mark pre-existing, unmanaged governance files as deployed when managed_files is None [governed-by-policy,secure-by-default]

• [nit] CHANGELOG entry may contain a stale editorial placeholder at CHANGELOG.md
  The added CHANGELOG line may contain author reminder text rather than release prose.

CLI Logging Expert

• [nit] Unmanaged-file detail lines surface a bare path but no remediation hint at src/apm_cli/policy/policy_checks.py:768
  The CheckResult.details list contains bare posix paths iterated verbatim in audit.py and policy_gate.py. Pre-existing gap, not introduced by this PR, but the new regression tests cement the current output contract, making a future improvement slightly harder to slip in quietly.
  Suggested: Add a trailing action hint to each detail line, e.g. unmanaged.append(rel + ' # add to a managed package or exclude in apm.yml policy'). The new tests would need updating too, but that is a small cost for better UX.

DevX UX Expert

• [recommended] Unmanaged-files diagnostic lists file paths but offers no remediation hint at src/apm_cli/policy/policy_checks.py:767
  When apm audit flags unmanaged files the user sees only a count + path list with no hint about which plugin to install or suggested apm install invocation. By the package-manager mental model an actionable error always names the next command.
  Suggested: Append a one-line remediation hint to the CheckResult details: 'Run apm install <plugin> to adopt a file, or add it to local_deployed_files in apm.lock.yaml to mark it as locally managed.'
  Proof (missing at unit): tests/unit/policy/test_policy_checks.py -- proves: When unmanaged files are detected the diagnostic message includes a concrete remediation step, not just a file list. [devx,governed-by-policy]

• [nit] Scan-cap silently passes the check instead of warning the user at src/apm_cli/policy/policy_checks.py:734
  When file scan exceeds _MAX_UNMANAGED_SCAN_FILES the check returns passed=True. CI sees a green check and never knows governance directories were too large to scan.
  Suggested: Escalate the message to stderr or use a distinct status='skipped' field so CI surfaces can surface it.

Supply Chain Security Expert

• [recommended] Scan-cap fail-open path is regression-pinned as correct behavior, making the bypass permanent at tests/unit/policy/test_policy_checks.py:631
test_rglob_cap_skips_check asserts result.passed=True when file count exceeds _MAX_UNMANAGED_SCAN_FILES. An adversary with write access to a governance directory can create N+1 files to trigger the cap and silence the unmanaged-files audit entirely. Pinning this as expected outcome raises the cost of ever making the cap fail-closed.
  Suggested: Change the cap behavior to emit a blocking result with a 'scan-capped' detail that CI can gate on.
  Proof (passed at unit): tests/unit/policy/test_policy_checks.py::TestUnmanagedFiles::test_rglob_cap_skips_check -- proves: The scan-cap path silently passes the security check when governance directories are large, with no blocking signal. [secure-by-default,governed-by-policy]

• [recommended] Unmanaged-files audit for .github/hooks/ has no integration-with-fixtures coverage
  The .github/hooks/ governance directory is a code-execution surface. All 5 regression tests are unit-tier (tmp_path, no real apm install subprocess). No test verifies the audit fires during an actual apm install invocation against a fixture repo.
  Suggested: Add an integration-with-fixtures test that runs apm install against a temp repo containing a hand-rolled .github/hooks/ file absent from any dependency's deployed_files, and asserts the command exits non-zero.
  Proof (missing at integration-with-fixtures): tests/unit/policy/test_policy_checks.py -- proves: The unmanaged-files audit reliably blocks install when .github/hooks/ contains a file not deployed by any plugin. [secure-by-default,governed-by-policy]

• [nit] test_local_deployed_files_not_flagged does not assert the negative case for _SELF_KEY exemption at tests/unit/policy/test_policy_checks.py:745
  Only the happy path is tested. A negative test -- listing a path NOT on disk -- would confirm the exemption is not a blanket allowlist.

OSS Growth Hacker

• [nit] CHANGELOG entry frames this as a test hardening story, not a trust guarantee for end users at CHANGELOG.md
  The current line reads 'regression tests prove hand-rolled files...are correctly flagged' -- this is maintainer language. Users care that the audit they rely on is trustworthy, not that tests exist.
  Suggested: 'Fix: unmanaged-files audit now correctly flags hand-rolled governance files that were silently treated as managed -- preventing false-clean audit results (closes [bug] apm install integrates files in .github/instructions/ that are not produced by an installed plugin #1199).'

Auth Expert -- inactive

PR only touches CHANGELOG.md and tests/unit/policy/test_policy_checks.py -- no auth, token management, credential resolution, or host classification surfaces are affected.

Doc Writer

• [recommended] CHANGELOG entry describes the regression tests, not the fix -- violates 'what changed for users' contract of the Fixed section at CHANGELOG.md
  The Fixed section answers 'what behavior changed'. The current entry -- 'regression tests prove hand-rolled files in governance directories are correctly flagged' -- describes evidence, not the fix. Users scanning the changelog cannot tell that apm install previously adopted unmanaged files.
  Suggested: - Fix unmanaged-files audit regression: apm install no longer adopts files in governance directories that were not produced by installed plugins; regression tests added to prevent recurrence. (closes #1199)

• [nit] Entry cites PR fix: apm install no longer adopts files not produced by installed plugins (closes #1199) #1256 but the fix closes issue [bug] apm install integrates files in .github/instructions/ that are not produced by an installed plugin #1199 -- cross-reference should point at the issue at CHANGELOG.md
  Every other Fixed entry cites the issue number (closes #NNN). Readers following #1256 land on the PR, not the original bug report.
  Suggested: Replace (#1256) with (closes #1199) at the end of the entry.

• [nit] Policy docs page worth a 30-second author check to confirm unmanaged_files behavior description is accurate post-fix at docs/src/content/docs/enterprise/policy-reference.md
  If the docs described the intended (correct) behavior all along, no change is needed. But if they implied any tolerance for hand-rolled files being adopted, a one-sentence clarification would close the gap.

Test Coverage Expert

• [nit] S7 run-probe skipped: pytest unavailable in runner; test correctness assessed by code read only
  Five regression tests in TestUnmanagedFilesPolicy cover the [bug] apm install integrates files in .github/instructions/ that are not produced by an installed plugin #1199 scenarios correctly. The implementation in policy_checks.py looks correct. However, pytest was not available in the runner environment, so test outcomes are unknown rather than confirmed. All 5 tests look structurally correct and should pass against the current implementation.
  Proof (unknown at unit): tests/unit/policy/test_policy_checks.py -- proves: Five regression tests for [bug] apm install integrates files in .github/instructions/ that are not produced by an installed plugin #1199 cover instruction flagging, sibling masking, cross-directory detection, local_deployed_files exemption, and trailing-slash prefix scoping. [governed-by-policy]

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

Generated by PR Review Panel for issue #1256 · ● 4.4M · ◷