[Test Improver] test: add unit tests for install/sources.py DependencySource classes

[danielmeppiel]

🤖 This PR was created by Test Improver, an automated AI assistant focused on improving test coverage for this repository.

Goal and Rationale

src/apm_cli/install/sources.py (595 lines) implements the Strategy pattern for the install pipeline — LocalDependencySource, CachedDependencySource, and FreshDependencySource — but only _format_package_type_label had tests (3 tests in test_sources_classification.py). The acquire() methods and the factory function had zero coverage.

These classes handle:

• Local (`(redacted) package copies with user-scope guards
• Cache-hit reuse with lockfile bookkeeping
• Fresh network downloads with supply-chain hash verification (sys.exit on mismatch)

The hash-mismatch abort path is a security-critical invariant; a regression there would silently accept tampered packages.

Approach

31 focused unit tests across 6 test classes:

Class	Tests	What is exercised
TestMaterialization	2	dataclass construction and custom deltas
TestMakeDependencySource	6	factory routing for all three source types
TestLocalDependencySourceAcquire	5	user-scope skip, copy failure, apm.yml present/absent
TestCachedDependencySourceAcquire	7	no-targets early return, pinned/unpinned delta, full acquire
TestFreshDependencySourceAcquire	8	download exception, hash mismatch abort, hash match pass, update_refs bypass, pre-downloaded results
TestIntegrateErrorPrefix	3	per-source error prefix constants

Heavy mocking of InstallContext keeps tests fast and isolated from network/filesystem concerns.

Coverage Impact

Component	Before	After
install/sources.py (_format_package_type_label only)	~5%	~65%+ (DependencySource classes)
Test count	5,405	5,436 (+31)

Trade-offs

• Uses MagicMock for InstallContext — tests are isolated but don't exercise the full pipeline. Integration tests in tests/integration/ cover the end-to-end flow.
• Hash-mismatch test uses pytest.raises(SystemExit) which is the correct way to assert sys.exit(1).

Test Status

tests/unit/install/test_dependency_sources.py  31 passed
Full unit suite:  5436 passed, 1 warning (pre-existing test_policy_status.py failure unrelated to this PR)

Reproducibility

uv sync --extra dev
uv run pytest tests/unit/install/test_dependency_sources.py -v
# Full suite (excluding pre-existing failure):
uv run pytest tests/unit tests/test_console.py --ignore=tests/unit/commands/test_policy_status.py -q

Generated by Daily Test Improver · ◷

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/daily-test-improver.md@b87234850bf9664d198f28a02df0f937d0447295

[github-actions]

APM Review Panel Verdict

Disposition: APPROVE (with one pre-merge fix -- assert safe_rmtree called in the hash-mismatch test)

---

Per-persona findings

Python Architect: This is a routine test addition (716 lines, 31 tests) for src/apm_cli/install/sources.py. The module implements the Strategy pattern for the install pipeline acquire phase; the PR exercises all three concrete strategies plus the factory.

1. OO / class diagram

classDiagram
    direction LR
    class DependencySource {
        <<Strategy>>
        +ctx InstallContext
        +dep_ref Any
        +install_path Path
        +dep_key str
        +acquire() Materialization
    }
    class LocalDependencySource {
        <<ConcreteStrategy>>
        +INTEGRATE_ERROR_PREFIX str
        +acquire() Materialization
    }
    class CachedDependencySource {
        <<ConcreteStrategy>>
        +INTEGRATE_ERROR_PREFIX str
        +resolved_ref Any
        +dep_locked_chk Any
        +acquire() Materialization
    }
    class FreshDependencySource {
        <<ConcreteStrategy>>
        +resolved_ref Any
        +dep_locked_chk Any
        +ref_changed bool
        +progress Any
        +acquire() Materialization
    }
    class Materialization {
        <<ValueObject>>
        +package_info PackageInfo
        +install_path Path
        +dep_key str
        +deltas Dict
    }
    DependencySource <|-- LocalDependencySource
    DependencySource <|-- CachedDependencySource
    DependencySource <|-- FreshDependencySource
    DependencySource ..> Materialization : returns
    note for DependencySource "Strategy: Local | Cached | Fresh\nmake_dependency_source() is the factory"
    class LocalDependencySource:::touched
    class CachedDependencySource:::touched
    class FreshDependencySource:::touched
    class Materialization:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

2. Execution flow diagram

flowchart TD
    A["make_dependency_source(ctx, dep_ref, ...)"] --> B{dep_ref.is_local?}
    B -->|Yes| C["LocalDependencySource.acquire()"]
    B -->|No + skip_download| D["CachedDependencySource.acquire()"]
    B -->|No + needs download| E["FreshDependencySource.acquire()"]
    C --> F{ctx.scope == USER?}
    F -->|Yes| G["diagnostics.warn + return None"]
    F -->|No| H["[FS] _copy_local_package(dep_ref, install_path)"]
    H -->|None| I["diagnostics.error + return None"]
    H -->|Path| J["[FS] build PackageInfo -> Materialization"]
    D --> K{ctx.targets empty?}
    K -->|Yes| L["return Materialization(package_info=None)"]
    K -->|No| M["[FS] load apm.yml -> PackageInfo -> Materialization"]
    E --> N["[NET] build_download_ref + downloader.download_package()"]
    N -->|Exception| O["diagnostics.error + return None"]
    N -->|Success| P{hash check: update_refs=False AND content_hash set?}
    P -->|Mismatch| Q["[FS] safe_rmtree + _rich_error + sys.exit(1)"]
    P -->|Match or skipped| R{ctx.targets empty?}
    R -->|Yes| S["return Materialization(package_info=None)"]
    R -->|No| T["return Materialization(package_info=pkg_info)"]

Loading

3. Design patterns

Design patterns

• Used in this PR: Strategy -- DependencySource ABC with 3 concrete strategies; tests exercise each branch via both the factory (TestMakeDependencySource) and direct construction.
• Used in this PR: Factory -- make_dependency_source() dispatches to the correct strategy; test_local_dep_takes_priority_over_skip_download verifies the priority ordering.
• Pragmatic suggestion: Fixture consolidation -- _make_dep_ref() and _make_ctx() are module-level helpers today; promoting them to tests/unit/install/conftest.py would make them reusable for future test_template.py / test_pipeline.py coverage with zero duplication cost.

Issue (cosmetic, follow-up): test_copy_failure_returns_none_and_records_error has a dead outer with patch(...) that conditionally targets LocalDependencySource.acquire.__wrapped__ (which never exists). The inner patch is the one that actually works; the outer conditional is dead code that muddies intent. No functional impact, but should be cleaned up.

---

CLI Logging Expert: Test-only PR -- no logging paths are modified. Key observations:

• test_user_scope_verbose_logger_called correctly asserts logger.verbose_detail.assert_called_once() -- aligned with APM's verbose-only detail principle.
• test_logger_called_on_cache_hit verifies logger.download_complete(..., cached=True) with a dual kwargs/positional check. Harmless but mildly over-defensive.
• Pre-existing antipattern (not introduced here): when logger is None in FreshDependencySource.acquire(), the code falls back to _rich_success() directly (sources.py:482). This violates the "route output through CommandLogger, not direct _rich_*" rule. No tests cover this fallback path in the PR, and no fix is required from this PR, but it is worth a follow-up issue.

No logging concerns introduced by this PR.

---

DevX UX Expert: Pure test PR -- no CLI command surface, flags, help text, or docs/reference/cli-commands.md changes needed.

The tests implicitly confirm good UX behaviors that are worth preserving:

• User-scope skip diagnostic includes a concrete next action: "Use a remote reference (owner/repo) instead" -- good.
• Hash mismatch error includes: "Use apm install --update to accept new content" -- good.

Minor: the assertion assert "org/repo" in err_msg or "Failed to install" in err_msg in test_download_exception_returns_none_and_records_error is over-permissive; either string alone would satisfy it. Tightening to assert "Failed to install" in err_msg alone would be more precise, but this is cosmetic.

No UX concerns.

---

Supply Chain Security Expert: This PR directly exercises the supply-chain hash verification boundary -- a critical path. Coverage is well-targeted.

Strengths:

• test_hash_mismatch_calls_sys_exit: verifies the abort-on-tamper path; safe_rmtree is patched to prevent filesystem side effects; pytest.raises(SystemExit) with code == 1 asserted.
• test_update_refs_skips_hash_verification: explicitly documents the intentional bypass in update mode. Important: this is a known escape hatch and having it tested prevents accidental re-enablement.
• test_hash_match_does_not_exit: confirms the happy path is not blocked.

Required gap: test_hash_mismatch_calls_sys_exit patches safe_rmtree but never asserts it was called. safe_rmtree is the step that removes the poisoned content from disk before aborting. If that call were accidentally deleted from FreshDependencySource.acquire(), the test would still pass while leaving tampered content on disk for the next install. Fix: capture the mock and add mock_rmtree.assert_called_once_with(install_path, ctx.apm_modules_dir).

Optional gap: No test for the case where dep_locked_chk.content_hash is set but dep_key not in ctx.package_hashes (line 511 guard in sources.py). In this case the hash check is silently skipped. Low risk, but an explicit test would document the behavior.

---

Auth Expert: Not activated -- PR adds tests for install/sources.py only; none of the fast-path auth files (auth.py, token_manager.py, azure_cli.py, github_downloader.py, marketplace/client.py, github_host.py, install/validation.py, registry_proxy.py) are modified, and no authentication behavior is changed. The test mocks ctx.auth_resolver as a MagicMock but does not test any auth resolution logic.

---

OSS Growth Hacker: Test-only PR; no conversion surface is touched. No WIP/growth-strategy.md update needed.

Side-channel note to CEO: explicit test coverage of the supply-chain abort path (test_hash_mismatch_calls_sys_exit) is an enterprise trust signal. When security-conscious evaluators audit APM's maturity, a test that asserts sys.exit(1) on hash mismatch is exactly the kind of proof that moves procurement conversations. Worth a one-liner in release notes when bundled with other quality PRs: "APM now has explicit unit coverage of supply-chain abort paths."

---

CEO arbitration

Specialists are aligned on both the value and the gap. This is a focused, well-scoped test addition that covers a genuinely important security boundary (hash-mismatch abort) and the full factory dispatch matrix. The one required fix is concrete and small: test_hash_mismatch_calls_sys_exit must assert that safe_rmtree is called, because a test that patches a critical cleanup step without asserting it ran does not fully protect against the regression it was written to catch. The cosmetic issues (double-patch, over-permissive OR assertion, direct _rich_success fallback in the source) are tracked as follow-ups and do not block merge. Disposition ratified: APPROVE with one pre-merge fix.

---

Required actions before merge

1. tests/unit/install/test_dependency_sources.py -- test_hash_mismatch_calls_sys_exit: Capture the safe_rmtree mock and assert it was called. The current test patches safe_rmtree but never asserts the call, meaning the disk-cleanup step could be silently removed without breaking the test. Suggested fix:

   with patch("apm_cli.drift.build_download_ref", return_value=MagicMock()), \
        patch.object(ctx.downloader, "download_package", return_value=fake_pkg_info), \
        patch("apm_cli.utils.content_hash.compute_package_hash", return_value="different_hash"), \
        patch("apm_cli.utils.path_security.safe_rmtree") as mock_rmtree, \
        patch("apm_cli.install.sources._rich_error"), \
        pytest.raises(SystemExit) as exc_info:
       src.acquire()
   assert exc_info.value.code == 1
   assert mock_rmtree.call_count == 1

---

Optional follow-ups

• Remove the dead outer with patch(...) conditional in test_copy_failure_returns_none_and_records_error; only the inner patch at apm_cli.install.phases.local_content._copy_local_package is needed.
• Add a test for the silent-skip case where dep_locked_chk.content_hash is set but dep_key not in ctx.package_hashes (documents the guard at sources.py:511).
• Promote _make_dep_ref() and _make_ctx() to tests/unit/install/conftest.py for reuse by future source-adjacent test files.
• Open a follow-up issue for the direct _rich_success() call in FreshDependencySource.acquire() when logger is None (sources.py:482); this bypasses CommandLogger and should be refactored to route through the logger lifecycle.

Generated by PR Review Panel for issue #927 · ● 768.6K · ◷