fix: scope install security scan to deployable content

[Aryan Singh K. (aryansk)]

Closes #2490

Summary

• allow SecurityGate.scan_files callers to restrict which paths are scanned
• scope the install pre-deploy blocker to deployable APM/plugin primitive paths instead of the entire fetched source tree
• clarify that a failed install may remove the fetched source checkout
• add regression coverage separating deployable content from source-only fixtures/docs

Validation

• patch syntax and application context were checked against current main
• repository tests could not be executed in this chat environment; CI should run the project test suite

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR narrows the pre-deploy security scan to “deployable” package content by introducing a path filter in the security gate and defining deployable-path heuristics, with accompanying unit tests.

Changes:

• Add an optional path_filter callback to SecurityGate.scan_files to skip scanning non-matching files.
• Define _is_deployable_source_path and deployable path patterns used by the install pre-deploy scan.
• Add unit tests covering deployable vs source-only paths and Windows path normalization.

Reviewed changes

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

File	Description
tests/unit/install/test_security_scan_scope.py	Adds tests for deployable-path detection logic and path normalization.
src/apm_cli/security/gate.py	Adds path_filter to optionally exclude files from scanning.
src/apm_cli/install/helpers/security_scan.py	Implements deployable path heuristics and wires filtering into the pre-deploy scan.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/apm_cli/security/gate.py:90

• scan_files() docstring says "scan every regular file" / "All files are scanned" but the new path_filter can intentionally skip files. Updating the docstring to describe the filter contract (and that the filter receives the portable forward-slash relpath) will prevent API confusion for future callers.

        """Walk *root*, scan every regular file, return a verdict.

        Symlinks are never followed (``followlinks=False``, ``is_symlink()``).
        All files are scanned to produce a complete findings report.
        """

tests/unit/install/test_security_scan_scope.py:29

• These tests validate the path classifier, but they don't assert that SecurityGate.scan_files(..., path_filter=...) actually prevents a critical finding in a non-deployable file from blocking an install. Adding a regression test that creates both a deployable file and a source-only file with a critical hidden char and asserts the source-only file is excluded (via verdict.scanned_files and verdict.should_block) would protect the end-to-end behavior this PR relies on.

from apm_cli.install.helpers.security_scan import _is_deployable_source_path


def test_deployable_primitives_are_scanned():
    deployable = [
        "SKILL.md",
        "plugin.json",
        ".apm/agents/reviewer.agent.md",
        ".github/instructions/security.instructions.md",
        "skills/review/SKILL.md",
        "hooks/hooks.json",
    ]

    assert all(_is_deployable_source_path(path) for path in deployable)


def test_source_only_files_are_not_scanned_by_install_gate():
    source_only = [
        "src/DotnetInspector.HostileNameFixtures/HostileLiterals.cs",
        "tests/fixtures/hostile-name.txt",
        "docs/hostile-metadata.md",
        "src/Program.cs",
    ]

    assert not any(_is_deployable_source_path(path) for path in source_only)


def test_windows_paths_are_normalized():
    assert _is_deployable_source_path(r".github\agents\reviewer.agent.md")

src/apm_cli/install/helpers/security_scan.py:49

• _is_deployable_source_path currently treats any path containing a deployable directory name anywhere in the path as deployable (e.g., any file under ".github"), which can still block installs due to findings in non-deployed areas like ".github/workflows" or nested source fixtures (e.g., "src/skills/..."), undermining the goal of scoping the install gate to deployable content only. Consider restricting the match to top-level deploy roots (".apm", ".github", "skills", "hooks", etc.) and, for ".apm"/".github", only allow known deployable subdirectories.

def _is_deployable_source_path(relative_path: str) -> bool:
    """Return whether a fetched source file belongs to deployable package content."""
    parts = tuple(part for part in relative_path.replace("\\", "/").split("/") if part)
    if not parts:
        return False
    if parts[-1] in _DEPLOYABLE_NAMES or parts[-1].endswith(_DEPLOYABLE_SUFFIXES):
        return True
    return any(part in _DEPLOYABLE_DIRS for part in parts[:-1])

[Sergio Sisternes (sergio-sisternes-epam)]

APM Review Panel: needs_rework

Scopes install-time security scan to deployable primitives only, eliminating false positives from test fixtures -- but the allowlist misses deployed executables.

cc Aryan Singh K. (@aryansk) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The core contribution -- a composable path_filter callback threaded into SecurityGate.scan_files -- is architecturally sound and the predicate pattern is clean. However, the supply-chain-security-expert's blocking finding is substantive and I side with it: bin/, canvas/, and MCP-server paths are provably deployed by integrate_package_primitives yet absent from _DEPLOYABLE_DIRS. This is not a theoretical gap; it is a concrete regression from the prior full-tree scan. A malicious package placing a hostile script in bin/ would evade the narrowed scan and still be deployed. On a secure-by-default surface this is blocking.

The doc-writer's blocking finding (FAQ line claiming 'all source files' are scanned) is real drift that ships user-facing misinformation on a security promise. It must land in the same PR or an immediate follow-up before merge.

The test-coverage gaps are notable: no integration test proves path_filter is actually wired into the install pipeline, and no test exercises the false-positive-avoidance scenario end-to-end. On a security-critical surface, missing wiring tests are near-blocking -- they are the only guarantee the filter survives refactoring. The UX regression on the 'may remove checkout' note is minor but real; it should be addressed but does not gate merge.

Dissent. Supply-chain-security rates _DEPLOYABLE_DIRS omission as blocking; no other panelist disputes this. I agree -- the allowlist MUST be a superset of deployed paths. The doc-writer's blocking finding is undisputed. The devx-ux 'recommended' on the cleanup note and supply-chain's 'recommended' on over-broad matching are both valid but non-blocking given the security fix takes priority.

Aligned with: secure-by-default: VIOLATED -- narrowing the scan without including all deployed paths (bin/, canvas/, MCP) weakens the default security posture; pragmatic-as-npm: ALIGNED in intent -- eliminating false positives from test fixtures reduces install friction; governed-by-policy: AT RISK -- docs now misstate the scan scope, which breaks the trust contract with enterprise adopters.

Growth signal. PR #2598 scopes security scan to deployable files only, eliminating false-positives from test fixtures. Worth a one-liner in next release notes: 'Security scans now target only deployable content -- no more false positives from test fixtures.' Hold the note until the allowlist gap is closed.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	Clean, well-scoped change. The filter pattern (pass a callable predicate into the scanner) is the correct minimal extension -- composition over subclassing. One type-annotation gap on the public API; otherwise architecturally sound.
CLI Logging Expert	0	0	1	Minor wording nit on the new tree-item note; no blocking issues with the CLI output changes.
DevX UX Expert	0	1	2	Blocked-install message regresses: noun label replaces actionable verb, and new cleanup note adds anxiety without guidance. Scope narrowing itself is fine but invisible.
Supply Chain Security Expert	1	2	0	Scan narrowing excludes bin/ and canvas paths that are still deployed -- creates executable bypass. Blocking.
OSS Growth Hacker	0	0	1	Good friction-removal for package authors; minor opportunity to convert the error path into a trust signal via docs link.
Doc Writer	1	3	0	3 doc pages have stale scan-scope claims (blocking FAQ, recommended inline); CHANGELOG [Unreleased] missing fix entry.
Test Coverage Expert	0	2	1	Unit predicate tests present; no test proves path_filter is wired into the install pipeline or exercises the false-positive-avoidance end-to-end.
Performance Expert	0	0	2	Filter is O(depth) per file with frozenset lookups; no algorithmic regression. Adds <25ms for 10k files. Ship as-is.

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

Top 5 follow-ups

1. [Supply Chain Security Expert] (blocking-severity) Expand _DEPLOYABLE_DIRS to include bin/, canvas/, and MCP-server paths -- everything integrate_package_primitives can deploy -- bin/, canvas/, and MCP paths are absent from the allowlist yet deployed; hostile executables bypass the scan entirely.
2. [Doc Writer] (blocking-severity) Update docs/src/content/docs/enterprise/security.md FAQ line 531 to say 'deployable primitive files' instead of 'all package source files' -- stale security claim in user-facing docs erodes enterprise trust.
3. [Test Coverage Expert] Add integration test proving path_filter is wired into the install pipeline and that non-deployable hostile content is skipped -- missing wiring test on a secure-by-default surface; the filter could be silently disconnected in a future refactor with no signal.
4. [DevX UX Expert] Drop or rephrase 'failed install may remove this checkout' note to an actionable hint -- adds anxiety without guidance on a failure path where clarity is paramount.
5. [Supply Chain Security Expert] Tighten directory-name matching to prevent over-broad ancestor matches (e.g. vendor/test-data/hooks/payload.md) -- current any(part in _DEPLOYABLE_DIRS ...) logic matches too broadly; reduces filter precision.

Architecture

classDiagram
    direction LR
    class SecurityGate {
      <<Facade>>
      +scan_files(root, policy, force, path_filter) ScanVerdict
      +report(verdict, diagnostics, package, force)
    }
    class ScanPolicy {
      <<ValueObject>>
      +block_on_critical bool
    }
    class ScanVerdict {
      <<ValueObject>>
      +has_findings bool
      +should_block bool
      +findings list
    }
    class ContentScanner {
      <<Utility>>
      +scan_file(path) list
    }
    class DiagnosticCollector {
      <<Collect-then-render>>
      +add_diagnostic(diag)
    }
    SecurityGate ..> ScanPolicy : uses
    SecurityGate ..> ScanVerdict : returns
    SecurityGate ..> ContentScanner : delegates scanning
    SecurityGate ..> DiagnosticCollector : reports to
    note for SecurityGate "path_filter: Callable predicate\ninjected by caller to scope walk"
    class SecurityGate:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm install invoked"] --> B["_pre_deploy_security_scan(install_path)"]
    B --> C["_is_deployable_source_path filter built"]
    C --> D["SecurityGate.scan_files(root, path_filter=filter)"]
    D --> E{"os.walk(root)"}
    E --> F{"fpath.is_symlink?"}
    F -->|yes| E
    F -->|no| G{"path_filter(rel)?"}
    G -->|no skip| E
    G -->|yes| H["[I/O] ContentScanner.scan_file(fpath)"]
    H --> I{"more files?"}
    I -->|yes| E
    I -->|no| J["return ScanVerdict"]
    J --> K{"verdict.should_block?"}
    K -->|yes| L["logger.error + return False"]
    K -->|no| M["return True -- proceed with deploy"]

Loading

Recommendation

The pattern and predicate logic are sound, but the allowlist is incomplete on a security-critical surface: deployed executables (bin/, canvas/, MCP) bypass the narrowed scan. This is a regression from the prior full-tree scan. Expand _DEPLOYABLE_DIRS to be a strict superset of everything integrate_package_primitives deploys, fix the stale FAQ claim, and add one wiring integration test. Once those land, this is a clean ship with a good release-note story.

---

Full per-persona findings

Python Architect

• [recommended] path_filter parameter lacks type annotation at src/apm_cli/security/gate.py:84
  The path_filter parameter in SecurityGate.scan_files is untyped (path_filter=None). All public APIs in APM should have type hints per code quality standards. This should be path_filter: Callable[[str], bool] | None = None with the appropriate import.
  Suggested: path_filter: "Callable[[str], bool] | None" = None
• [nit] Deployable vocabulary is a candidate for centralization
  The _DEPLOYABLE_DIRS, _DEPLOYABLE_NAMES, and _DEPLOYABLE_SUFFIXES constants define what constitutes 'deployable content' -- a concept that may be needed by other subsystems. If a second call site emerges, extract to core/primitives.py per the single-owner rule. Fine as-is for one call site.
• [nit] Test imports a private function directly
  Importing _is_deployable_source_path couples tests to an internal name. If this filter is considered a stable contract worth testing independently, consider making it public or testing indirectly through _pre_deploy_security_scan.

CLI Logging Expert

• [nit] New tree-item note is passive and non-actionable
  The note about transaction cleanup is informational but gives the user nothing to act on. Consider shortening to |-- Checkout may be removed on rollback to stay terse, or drop it entirely since --force is the real action.

DevX UX Expert

• [recommended] New 'failed install may remove this checkout' note is confusing and non-actionable at src/apm_cli/install/helpers/security_scan.py:88
  The user just got a security block. The next-action they need is 'inspect' or '--force'. Injecting a note about transaction cleanup introduces a concept (checkout lifecycle) they didn't ask about, creates anxiety ('will my files disappear?'), and doesn't tell them what to DO.
  Suggested: Either drop the note entirely or rephrase: '|-- Hint: copy or inspect the checkout before retrying -- cleanup may remove it'
• [nit] Rename 'Source checkout' back to 'Inspect source' for verb-led scannability at src/apm_cli/install/helpers/security_scan.py:87
  'Inspect source: ' tells the user what to do (verb) and where (path). 'Source checkout: ' is a noun-noun label that requires the user to infer the action.
• [nit] Silent scope narrowing is invisible to users -- consider a verbose-mode breadcrumb
  When the scan passes, no output tells the user what was actually scanned. A --verbose or debug log line like 'Security scan: 12 deployable files checked' would preserve trust.

Supply Chain Security Expert

• [blocking] _DEPLOYABLE_DIRS omits bin, canvas, and MCP-server paths -- deployed executables bypass security gate at src/apm_cli/install/helpers/security_scan.py:14
_DEPLOYABLE_DIRS omits bin, canvas, and MCP-server paths, yet integrate_package_primitives deploys executables from bin/, canvas extensions, and MCP configs. A malicious package can place a hostile script in bin/malware.sh -- it will NOT be scanned but WILL be deployed and potentially executed. The filter allowlist MUST be a superset of everything integrate_package_primitives can deploy, or this creates a scan-bypass vector for the most dangerous artifact class (executables).
• [recommended] Directory-name-only matching is over-broad and under-specific at src/apm_cli/install/helpers/security_scan.py:44
any(part in _DEPLOYABLE_DIRS for part in parts[:-1]) matches ANY ancestor directory segment, not just the package-root-relative first segment. A path like vendor/test-data/hooks/payload.md passes the filter even though hooks/ is nested under unrelated directories.
• [recommended] path_filter lacks type annotation and contract documentation at src/apm_cli/security/gate.py:84
  Add path_filter: Callable[[str], bool] | None = None and a note that returning False SKIPS the file (fail-open semantics -- important to document for security-sensitive API).

OSS Growth Hacker

• [nit] Error message could link to docs explaining the security scan
  A package author hitting a false-positive block during development has high frustration. Adding a docs URL in the CLI output converts that frustration moment into trust.

Auth Expert -- inactive

PR touches security_scan.py and gate.py for file-path filtering of deployable content; no auth, token, credential, or remote-host surface is affected.

Doc Writer

• [blocking] FAQ answer now incorrect: 'scans all package source files' at docs/src/content/docs/enterprise/security.md:531
  The FAQ answer 'APM scans all package source files before deployment' is now incorrect. After this PR, only deployable primitive files are scanned. Should say: 'APM scans deployable primitive files in each package before deployment.'
• [recommended] Content scanning section implies all source files are scanned at docs/src/content/docs/enterprise/security.md:153
  The sentence implies full-repo scanning. After this PR the scan is scoped to deployable content. Adding 'deployable' before 'source files' would make it accurate.
• [recommended] Install description says 'scans every primitive' -- acceptable but imprecise at docs/src/content/docs/consumer/install-packages.md:8
  Both the intro and pipeline step 3 are directionally correct but loose. Consider tightening to 'scans deployable primitives in each package'.
• [recommended] Unreleased section missing entry for install scan scope fix at CHANGELOG.md:8
  The [Unreleased] block has no entry for this behavioral change. Users running apm install who previously saw false-positive scan blocks will notice the change. A Fixed entry closing [BUG] Install security scan flags hidden characters in files that are never deployed (repo-wide scan vs deployable primitives) #2490 belongs here.

Test Coverage Expert

• [recommended] No test asserts path_filter is passed to SecurityGate.scan_files
  The existing TestPreDeploySecurityScan tests all mock SecurityGate.scan_files, so they never verify that path_filter=_is_deployable_source_path is actually wired in. If someone removes the path_filter kwarg, no test fails.
  Proof (missing): tests/integration/test_install_marketplace_phase4w3.py::test_path_filter_wired_to_scan_files -- proves: The install pipeline actually passes the filter to the gate
• [recommended] No integration test proves hostile content in non-deployable paths is skipped
  No test exercises this end-to-end with real files on disk. The unit tests prove the predicate returns False for those paths, but the mocked integration tests never let a real file walk occur.
  Proof (missing): tests/integration/test_install_marketplace_phase4w3.py::test_hostile_fixture_in_non_deployable_path_not_blocked -- proves: A package with hostile test fixtures installs without being blocked
• [nit] SecurityGate.scan_files has no unit test exercising the path_filter callback
  gate.py now accepts path_filter and uses it at line 100. tests/unit/test_security_gate.py has no test for this new branch.
  Proof (missing): tests/unit/test_security_gate.py::test_scan_files_respects_path_filter -- proves: SecurityGate.scan_files honors path_filter callback

Performance Expert

• [nit] Per-file filter cost is negligible
  The filter does: one str.replace, one str.split, a tuple comprehension over path depth, a frozenset in check (O(1)), and an endswith against a small tuple. For a 10k-file tree this adds ~15-25ms total -- well under noise. No O(n^2) risk.
• [nit] Confirm collections are frozenset and tuple at module level
  If _DEPLOYABLE_DIRS is a frozenset and _DEPLOYABLE_SUFFIXES is a tuple (both at module scope), the per-call cost is optimal. Both are already correct in the diff.

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