refactor: complexity audit -- eliminate god-method forks, O(n^2) loops, redundant I/O by sergio-sisternes-epam · Pull Request #918 · microsoft/apm

sergio-sisternes-epam · 2026-04-24T22:16:40Z

Complexity Audit & Code Quality Improvements

Summary

Comprehensive codebase quality audit covering algorithmic improvements, god function decomposition, exception hygiene, and performance benchmarks.

Commits (10 total)

Commit	Description
Round 1 refactoring	6 algorithmic phases: lockfile dedup, console singleton, reverse-dep O(n^2)->O(n), discovery scan, registry cache, NullCommandLogger
Benchmarks (3 iterations)	160 tests: 154 benchmarks + 6 scaling guards covering install, compilation, security, resolver
WI-1: Exception hygiene	7 bare `except:` replaced with `except Exception:`, debug logging for silent handlers
WI-4a-d: God function splits	reference.py (110->22 stmts), audit.py (290->18+49+67), deps/cli.py (135->59), script_runner.py (124->20)
P1 test gaps (Round 2)	9 tests for ScriptExecutionFormatter + CLAUDE.md injection
WI-2: Downloader decomposition	`github_downloader.py` split into 3 modules: `git_remote_ops.py` (ref parsing), `download_strategies.py` (DownloadStrategyManager), slimmed orchestrator (2,669->2,068 lines)
WI-3: Install decomposition	`install()` 555-line god function split into dispatcher + `_install_apm_packages()` + `_handle_mcp_install()` + `_post_install_summary()` with `InstallContext` dataclass
P1 test gaps (WI-2/WI-3)	17 tests: InstallContext dataclass validation + `_resolve_package_references()` mutation contract

Findings resolved

ID	Severity	Finding	Resolution
B1	BLOCKING	`install()` 27-param/378-stmt god function	WI-3: decomposed into 4 focused helpers + InstallContext dataclass
B2	BLOCKING	`github_downloader.py` 2,669-line god file	WI-2: split into 3 modules with backward-compat stubs
B3	BLOCKING	Bare `except:` catches KeyboardInterrupt	WI-1: replaced with `except Exception:`
B4	BLOCKING	Silent auth fallback	WI-1: added `logger.debug()`
W2	WARNING	reference.py parse chain 110 stmts	WI-4a: split into 3 focused parsers
W3	WARNING	audit.py god function 290 lines	WI-4b: dispatcher + 2 handlers
W4	WARNING	deps/cli.py mixed data/rendering	WI-4c: separated resolution from display
W5	WARNING	script_runner.py runtime dispatch 124 stmts	WI-4d: per-runtime builders

Test results

5,441 unit tests passing (26 new tests for decomposition coverage)
160 benchmark tests (154 benchmarks + 6 scaling guards)
Zero regressions

Related issues

Audit: auth.py exception cascade -- 15 clauses need security-aware review #935: auth.py exception cascade audit (follow-up)
Security: policy_checks.py silently bypasses enforcement on malformed YAML #936: policy_checks.py silent policy bypass (follow-up)

Extract pure git ref helpers into git_remote_ops.py and backend download logic into download_strategies.py DownloadStrategyManager. Backward-compat method stubs on GitHubPackageDownloader preserve all existing import paths and test patch points. Part of complexity audit PR microsoft#918. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Split 555-line install() into thin dispatcher + _install_apm_packages() + _handle_mcp_install(). Extract _resolve_package_references() and _check_package_conflicts() from _validate_and_add_packages_to_apm_yml(). Add InstallContext dataclass to bundle shared parameters. Part of complexity audit PR microsoft#918. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Add tests for commands.install.InstallContext dataclass construction and _resolve_package_references() batch-duplicate mutation contract. Part of complexity audit PR microsoft#918. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Refactor-focused PR that reduces complexity and improves performance across install/audit/deps/uninstall paths, while adding benchmarks and characterization/unit tests to guard behavior and scaling.

Changes:

Introduces thread-safe caching/singletons (Rich console, marketplace registry cache) and reduces redundant I/O (lockfile reuse, per-runtime registry reads).
Replaces several O(n^2) loops with indexed/O(n) approaches (uninstall reverse-dep traversal, discovery scanning strategy).
Adds substantial benchmark/scaling-guard coverage plus characterization/unit tests for refactored components.

Reviewed changes

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

Show a summary per file

File	Description
tests/unit/test_uninstall_engine_helpers.py	Adds unit coverage for uninstall reverse-dependency children index helper.
tests/unit/test_transitive_mcp.py	Updates tests to match MCPIntegrator logging/output behavior changes.
tests/unit/test_thread_safety.py	Adds concurrency tests for console singleton and marketplace registry cache lock.
tests/unit/test_script_formatters.py	Adds tests for ScriptExecutionFormatter Rich fallback branches and formatting.
tests/unit/test_registry_integration.py	Adds tests ensuring MCP registry runtime lookups are cached per runtime.
tests/unit/test_mcp_integrator_remove_stale.py	Adds characterization coverage for MCPIntegrator.remove_stale() argument combinations.
tests/unit/test_mcp_integrator_coverage.py	Adds coverage tests for MCPIntegrator branches + NullCommandLogger surface.
tests/unit/test_mcp_integrator_characterisation.py	Adds install() characterization tests across logger/runtime/flags combinations.
tests/unit/test_console_utils.py	Resets console singleton between tests to avoid cross-test coupling.
tests/unit/install/test_architecture_invariants.py	Updates LOC budget gate for install.py after decomposition.
tests/unit/compilation/test_agents_compiler_coverage.py	Adds test for constitution injection failure being swallowed + debug-logged.
tests/unit/commands/test_install_resolve_refs.py	Adds tests for in-place identity-set mutation contract in resolver helper.
tests/unit/commands/test_install_context.py	Adds structural tests for commands.install.InstallContext dataclass.
tests/benchmarks/test_scaling_guards.py	Adds non-benchmark scaling guards to detect complexity regressions in CI.
tests/benchmarks/test_install_hot_paths.py	Adds benchmark suite for install hot paths (hashing, lockfile ops, flattening, etc.).
tests/benchmarks/test_git_and_compiler_benchmarks.py	Adds benchmarks for git ref parsing/sorting, compiler analysis, and MCP transitive collection.
tests/benchmarks/test_compilation_hot_paths.py	Adds compilation/integration benchmarks (hashing, partitioning, link rewrite, lockfile RTT).
tests/benchmarks/test_audit_benchmarks.py	Adds audit-focused benchmarks for discovery, uninstall indexing, registry caching, console singleton, etc.
src/apm_cli/utils/console.py	Implements thread-safe Rich Console singleton + test-only reset hook.
src/apm_cli/registry/operations.py	Caches installed server IDs per runtime to avoid O(servers*runtimes) config reads.
src/apm_cli/primitives/discovery.py	Refactors primitive discovery to single os.walk pass with pattern matching helpers.
src/apm_cli/policy/discovery.py	Improves exception hygiene by logging token manager failure at debug level.
src/apm_cli/output/script_formatters.py	Replaces bare `except:` with `except Exception:` in Rich fallback blocks.
src/apm_cli/output/formatters.py	Replaces bare `except:` with `except Exception:` in multiple formatting fallbacks.
src/apm_cli/models/dependency/reference.py	Splits parsing logic into smaller helpers; centralizes validation/final field extraction.
src/apm_cli/models/apm_package.py	Deduplicates dependency parsing for dependencies/devDependencies via helper.
src/apm_cli/marketplace/registry.py	Adds lock around registry cache to make load/save/invalidate thread-safe.
src/apm_cli/integration/mcp_integrator.py	Introduces NullCommandLogger default and removes many logger=None forks.
src/apm_cli/install/pipeline.py	Threads early lockfile through InstallContext to avoid re-reading.
src/apm_cli/install/phases/resolve.py	Uses cached early lockfile when available and improves verbose lockfile logging.
src/apm_cli/install/context.py	Adds early_lockfile field to install pipeline context.
src/apm_cli/deps/git_remote_ops.py	Extracts pure helpers for parsing/sorting `git ls-remote` output.
src/apm_cli/core/script_runner.py	Decomposes runtime command transformation and avoids redundant apm_modules scans.
src/apm_cli/core/null_logger.py	Adds NullCommandLogger (null-object) to centralize default logging behavior.
src/apm_cli/compilation/agents_compiler.py	Adds debug logging for previously silent exception paths.
src/apm_cli/commands/uninstall/engine.py	Adds O(n) children index and uses it to avoid repeated full scans.
src/apm_cli/commands/deps/cli.py	Splits data resolution from rendering; extracts helpers for tree formatting.
src/apm_cli/commands/audit.py	Decomposes audit command into mode handlers with shared config dataclass.
CHANGELOG.md	Adds Unreleased entries describing refactors, benchmarks, and exception hygiene updates.

github-actions · 2026-04-25T11:06:09Z

APM Review Panel Verdict

Disposition: REQUEST_CHANGES (two concrete pre-merge fixes; overall shape is excellent)

Per-persona findings

Python Architect:

This is a major architectural change: three new modules introduced (download_strategies.py, git_remote_ops.py, null_logger.py), a class hierarchy restructured around a delegation pattern, and two new parameter-bundle dataclasses (InstallContext in commands/install.py, _AuditConfig in audit.py). Before/After diagrams follow.

Before -- WI-2 class layout (monolith)

classDiagram
    direction LR
    class GitHubPackageDownloader {
        <<Monolith>>
        +resilient_get()
        +build_repo_url()
        +download_file()
        +download_artifactory_archive()
        +parse_ls_remote_output()
        +semver_sort_key()
        +sort_remote_refs()
        +github_token str
        +ado_token str
        +auth_resolver AuthResolver
    }
    class AuthResolver {
        <<Strategy>>
        +resolve_for_dep(dep_ref) AuthContext
    }
    GitHubPackageDownloader ..> AuthResolver : reads
    note for GitHubPackageDownloader "All download, URL-build, and ref-sort\nlogic inlined -- 1000+ line module"

After -- WI-2 class layout (decomposed)

classDiagram
    direction LR
    class GitHubPackageDownloader {
        <<Facade>>
        +github_token str
        +ado_token str
        +auth_resolver AuthResolver
        +_strategies DownloadStrategyManager
        +_resilient_get() -- stub
        +build_repo_url() -- stub
        +download_artifactory_archive() -- stub
    }
    class DownloadStrategyManager {
        <<ConcreteStrategy>>
        +_host GitHubPackageDownloader
        +resilient_get()
        +build_repo_url()
        +download_file()
        +download_artifactory_archive()
    }
    class git_remote_ops {
        <<Pure>>
        +parse_ls_remote_output()
        +semver_sort_key()
        +sort_remote_refs()
    }
    class NullCommandLogger {
        <<NullObject>>
        +verbose = False
        +warning()
        +progress()
        +error()
        +success()
    }
    class InstallContextCmds {
        <<ParameterBundle>>
        +scope
        +logger
        +force bool
        +dry_run bool
    }
    class _AuditConfig {
        <<ValueObject>>
        +project_root Path
        +logger CommandLogger
        +verbose bool
        +output_format str
    }
    GitHubPackageDownloader *-- DownloadStrategyManager : owns
    DownloadStrategyManager ..> GitHubPackageDownloader : back-ref (_host)
    GitHubPackageDownloader ..> git_remote_ops : imports
    class GitHubPackageDownloader:::touched
    class DownloadStrategyManager:::touched
    class git_remote_ops:::touched
    class NullCommandLogger:::touched
    class InstallContextCmds:::touched
    class _AuditConfig:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
    note for DownloadStrategyManager "Bidirectional coupling via _host --\nsee Required Actions"

Before -- WI-2/WI-3 execution flow

flowchart TD
    A["apm install (CLI)"] --> B["install() -- 555 lines"]
    B --> C["_validate_and_add_packages_to_apm_yml -- 290 lines"]
    C --> D["[I/O] load_yaml(apm.yml)"]
    D --> E["inline duplicate detection loop"]
    E --> F["inline package validation loop"]
    F --> G["[I/O] dump_yaml(apm.yml)"]
    G --> H["run_install_pipeline()"]
    H --> I["resolve.run() -- [I/O] LockFile.read() x2"]

After -- WI-2/WI-3 execution flow

flowchart TD
    A["apm install (CLI)"] --> B["install()"]
    B --> C["InstallContext -- commands/install.py"]
    C --> D["_validate_and_add_packages_to_apm_yml"]
    D --> D1["_check_package_conflicts()"]
    D --> D2["_resolve_package_references()"]
    D --> D3["_merge_packages_into_yml()"]
    D3 --> E["[I/O] dump_yaml(apm.yml)"]
    E --> F["run_install_pipeline()"]
    F --> G["install/context.py:InstallContext -- early_lockfile cached"]
    G --> H["resolve.run() -- [I/O] LockFile.read() x1 (cache hit skips)"]

Design patterns

Used in this PR:
- Facade -- GitHubPackageDownloader becomes a thin facade with backward-compat stubs delegating to DownloadStrategyManager
- Null Object -- NullCommandLogger eliminates 32 if logger: forks in MCPIntegrator
- Parameter Bundle (DTO) -- InstallContext (commands) and _AuditConfig reduce multi-argument function signatures to a single config object
- Pure Module -- git_remote_ops.py is stateless helpers with no side effects (<<Pure>> stereotype)
Pragmatic suggestion: DownloadStrategyManager._host is a bidirectional back-reference that keeps orchestrator and strategy coupled at the instance level. A thin _DownloaderConfig dataclass holding only github_token, ado_token, auth_resolver, and registry_config would sever the cycle without adding abstraction depth. At this PR's current size this is optional, not required.

Architecture issue -- dual InstallContext name: src/apm_cli/commands/install.py:161 defines InstallContext; src/apm_cli/install/context.py:116 also defines InstallContext. Both are used in the install pathway. The commands-layer one should be renamed (e.g. _AddPackagesParams) to prevent future import shadowing and make grep results unambiguous.

CLI Logging Expert:

NullCommandLogger is a clean Null Object that eliminates the 32 if logger: forks in MCPIntegrator. The methods it implements (warning, progress, error, success, verbose_detail, tree_item, package_inline_warning) match exactly the methods MCPIntegrator calls on its logger parameter -- no AttributeError risk in current usage.

One cosmetic bug: _merge_packages_into_yml (line 463) sets dep_label = "devDependencies" if dev else "apm.yml". The non-dev branch says "apm.yml" where it should say "dependencies". The verbose_detail message then reads "Added foo to apm.yml" instead of "Added foo to dependencies". Low severity (verbose-only), but misleading.

Audit command decomposition routes through CommandLogger correctly -- _audit_ci_gate and _audit_content_scan both receive the logger via cfg.logger and call .error(), .warning(), .success(), .progress() exclusively. No rogue _rich_* calls in command functions. Good.

DevX UX Expert:

This is a pure internal refactoring. No command surface, flags, help text, or exit codes are changed. The apm install, apm audit, apm update user experience is identical before and after.

The early_lockfile caching on InstallContext eliminates a redundant lockfile read -- users won't notice, but CI pipelines on large monorepos will see a small improvement.

No documentation changes are needed (no command surface delta). cli-commands.md and quick-start.md are unaffected.

One ergonomic note echoing the logging finding: a verbose user running apm install --verbose mypackage would see "Added mypackage to apm.yml" (bug) rather than "Added mypackage to dependencies". Trivial, but worth fixing before it lands.

Supply Chain Security Expert:

Reviewed against all seven threat-model categories. Key findings:

Zip path traversal guard preserved (threat 7): download_artifactory_archive moved to DownloadStrategyManager with the containment guard intact: if not dest.resolve().is_relative_to(target_path.resolve()). No regression.
Bearer token URL non-embedding preserved (threat 6): build_repo_url with auth_scheme == "bearer" still passes token=None to build_ado_https_clone_url and injects credentials via GIT_CONFIG env vars. Token never appears in a URL or log.
_strip_extended_prefix removal (src/apm_cli/utils/path_security.py): This was the Windows PyInstaller \\?\ prefix fix (Windows: _get_cache_dir flakes on tmpdir 8.3 short names (PathTraversalError) #886). Removing it is safe because the project targets Python 3.12 where Path.resolve() handles extended-length paths consistently. However, the removal carries no explanatory comment -- a future reader (or a contributor running Python 3.10/3.11) will not know why the guard was dropped. A one-line comment citing Windows: _get_cache_dir flakes on tmpdir 8.3 short names (PathTraversalError) #886 and Python 3.12 is a required action.
subprocess_env.py deletion: The module is fully unreferenced (confirmed: grep -r external_process_env src/ finds no hits). Deletion is clean. The PyInstaller LD_LIBRARY_PATH leakage issues ([BUG] Fedora: brew apm fails on Git clone due to bundled OpenSSL mismatch #462, [BUG] apm update fails #894) were addressed via a different mechanism upstream. No regression.
_resolve_package_references extraction: The insecure-URL guards and local-at-user-scope rejection appear correctly preserved in the extracted helper. No paths were silently removed.

Auth Expert:

Activated (fast-path trigger: src/apm_cli/deps/github_downloader.py changed).

DownloadStrategyManager.build_repo_url reads tokens via self._host.github_token and self._host.ado_token -- these are AuthResolver-resolved instance fields on the owning GitHubPackageDownloader, not direct os.getenv() calls. Token precedence chain is unchanged: all resolution still flows through AuthResolver before reaching the downloader instance.

The token == "" sentinel case (explicit empty string to suppress per-instance token for plain HTTPS/SSH transport attempts) is correctly handled in DownloadStrategyManager.build_repo_url at lines that set github_token = "" and ado_token = "". This is a deliberate transport-selector escape hatch and is preserved faithfully.

ADO bearer scheme: auth_scheme == "bearer" branch passes token=None to build_ado_https_clone_url(). Bearer tokens are injected separately via GIT_CONFIG env vars (not embedded in URL). This is correct per the auth architecture.

get_artifactory_headers() delegation: The backward-compat stub on GitHubPackageDownloader delegates to self._strategies.get_artifactory_headers(). The actual header construction uses self._host.registry_config or falls back to self._host.artifactory_token -- both read via the back-reference, same fields, same fallback chain. No credential-handling regression.

No new os.getenv token reads introduced. AuthResolver remains the sole credential source. No regression to AuthResolver precedence, host classification, or credential leakage surface.

OSS Growth Hacker:

No user-facing feature lands with this PR -- it is a pure engineering investment. Three growth implications worth noting:

Contributor funnel improvement (side-channel to CEO): Splitting github_downloader.py from ~1000 lines to three focused modules lowers the cognitive barrier for first-time contributors. The "god file" problem is a known contributor churn driver in OSS. This is a real, compounding gain.
Benchmark suite as enterprise trust signal: 145+ new benchmark tests covering hot paths is a credibility signal for enterprise evaluators. "APM instruments its own performance" is a one-liner that maps cleanly to the "production-grade package manager" positioning.
No conversion surface touched: README.md, quickstart, and first-run flow are unaffected. The _strip_extended_prefix removal is a potential regression on non-Python-3.12 Windows builds if a user runs APM from source under an older Python -- worth flagging to the CEO but not a launch-narrative risk for binary users.

Side-channel to CEO: the dual InstallContext naming is a future contributor confusion risk that could generate low-quality issues or incorrect PRs from contributors who import the wrong class. Worth fixing before the branch merges.

CEO arbitration

Specialists are broadly aligned: the engineering quality of this refactoring is high, patterns are correctly applied, and no auth or supply-chain regressions are introduced. Two issues rise to required-action level. First, _strip_extended_prefix was a deliberate security guard for PyInstaller + Windows; removing it without a comment creates a maintenance trap for any contributor who hits the Windows edge case, runs git bisect, and finds the guard gone with no explanation -- a one-line comment citing #886 costs nothing and prevents future confusion. Second, the duplicate InstallContext name across commands/install.py and install/context.py will produce hard-to-diagnose shadowing bugs once external contributors import from either module: rename the commands-layer class now while the PR is still open. The dep_label cosmetic bug ("apm.yml" vs "dependencies" in a verbose_detail message) is a required fix to keep verbose output trustworthy. These three changes are small, concrete, and unambiguous. The benchmark suite, NullCommandLogger, and O(n^2)->O(n) performance work are all ratified as mergeable; the code quality of WI-1 through WI-3 is a net positive for the project. Disposition: REQUEST_CHANGES on the three items below.

Required actions before merge

src/apm_cli/utils/path_security.py -- add a comment explaining _strip_extended_prefix removal. One line is enough: # _strip_extended_prefix removed: Python 3.12 Path.resolve() handles \\?\ prefixes correctly (see #886). Without it, a future contributor or security auditor will re-add the check (or question the omission in review) creating unnecessary churn.
src/apm_cli/commands/install.py:161 -- rename InstallContext to _AddPackagesParams (or similar). The install pathway now contains two distinct InstallContext dataclasses (commands/install.py:161 and install/context.py:116). Both are used in the same feature, both carry "install context" semantics, and both will appear in search results and IDE autocomplete. Rename the commands-layer one to remove ambiguity before this lands.
src/apm_cli/commands/install.py:463 -- fix dep_label value. Change "devDependencies" if dev else "apm.yml" to "devDependencies" if dev else "dependencies". The verbose_detail message currently tells users their package was added "to apm.yml" instead of "to dependencies".

Optional follow-ups

DownloadStrategyManager._host back-reference: consider introducing a _DownloaderConfig dataclass holding only github_token, ado_token, auth_resolver, and registry_config -- injected into both GitHubPackageDownloader and DownloadStrategyManager to break the bidirectional coupling. Not required at this scope but would simplify future unit-testing of the strategy manager in isolation.
NullCommandLogger docstring: narrow "drop-in replacement for CommandLogger" to "drop-in for contexts where only warning/progress/error/success are called (e.g. MCPIntegrator)". The current docstring overclaims: CommandLogger has additional phase methods (lockfile_entry, validation_start, etc.) that NullCommandLogger does not implement. Callers outside MCPIntegrator that use the full InstallLogger interface would get AttributeError.
Add a dated entry to WIP/growth-strategy.md (maintainer-local, gitignored): "WI-1/2/3 complexity audit reduces contributor onboarding friction -- link the module split in next release notes as evidence of engineering maturity."

Generated by PR Review Panel for issue #918 · ● 1M · ◷

- Route user-visible MCP messages through logger.progress() instead of verbose_detail() so NullCommandLogger preserves output - Fix trivially-passing frozen assertion in test_install_context.py - Add missing (microsoft#918) PR references to CHANGELOG entries Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>