refactor: tighten stage 1 ruff thresholds

[zeel2104]

Description

Tightens the Stage 1 Ruff complexity thresholds for the strangler-fig refactor work and trims the main outliers blocking those guardrails.

Changes include:

• Reduced complexity and branching in MCPIntegrator.install by extracting runtime selection, registry install, self-defined install, and summary helpers.
• Reduced return pressure in marketplace publishing by splitting single-target processing into smaller checkout/load/guard/write/commit/push helpers.
• Reduced install wrapper argument count with a frozen options dataclass.
• Added targeted waivers for existing legacy helper functions where broader extraction is better suited to follow-up stages.
• Updated Ruff thresholds in pyproject.toml to the Stage 1 targets.

Fixes #1077

Type of change

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

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

Validation run:

uv run --python 3.13 --extra dev ruff check src/ tests/


## Results

All checks passed

[Copilot]

Pull request overview

Refactors several high-complexity/return-pressure hotspots to meet Stage 1 Ruff thresholds, and updates the Ruff thresholds in pyproject.toml accordingly (per #1077).

Changes:

• Updates Ruff complexity/args/returns thresholds to Stage 1 targets in pyproject.toml.
• Extracts helper functions to reduce complexity/returns in MCP installation and marketplace publishing flows.
• Reduces install wrapper argument pressure by introducing an options dataclass and **kwargs validation.

Reviewed changes

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

Show a summary per file

File	Description
src/apm_cli/policy/policy_checks.py	Refactors check execution into iterable loops for dependency + MCP policy checks.
src/apm_cli/marketplace/semver.py	Simplifies comparison operator handling via a small operator/comparator table.
src/apm_cli/marketplace/publisher.py	Splits per-target publish processing into smaller checkout/load/guard/write/commit/push helpers.
src/apm_cli/integration/mcp_integrator.py	Extracts runtime selection, dependency splitting, registry/self-defined install, and summary helpers to reduce install complexity.
src/apm_cli/install/validation.py	Adds targeted Ruff waivers for legacy complexity outliers.
src/apm_cli/install/services.py	Adds targeted Ruff waiver for max-args on a legacy entry point.
src/apm_cli/install/mcp/conflicts.py	Adds targeted Ruff waiver for max-args on a legacy validator.
src/apm_cli/install/mcp/command.py	Adds targeted Ruff waiver for max-args on a legacy command wrapper.
src/apm_cli/commands/install.py	Introduces frozen options dataclass for _install_apm_dependencies and switches wrapper to **kwargs.
pyproject.toml	Tightens Ruff thresholds to Stage 1 values (statements/branches/complexity/args/returns).

[sergio-sisternes-epam]

Hey @zeel2104 -- welcome and thanks for picking up #1077! The strangler-fig direction here is exactly right, and the threshold reductions are meaningful. Great first contribution.

I can see that commit 71784c5 already addresses all four Copilot review comments (lazy lambdas for fail_fast, Protocol-based type hints, Optional annotations on the dataclass). Nice work -- please reply to each Copilot comment thread explaining the fix and resolve the conversations so the review state is clean.

There are still a few CI blockers that need addressing before we can proceed:

CI failures

1. Ruff format -- mcp_integrator.py and publisher.py need reformatting. Running uv run --extra dev ruff format src/ tests/ should sort this out.
2. test_install_py_under_legacy_budget -- commands/install.py is now at ~1840 LOC, which exceeds the 1825-line architecture guardrail. This needs a proper extraction into apm_cli/install/ rather than cosmetic trimming (the test message itself points to the python-architecture skill for guidance).
3. test_no_stored_configs_preserves_existing_behavior -- the MCP integrator refactor appears to have moved or renamed the _rich_success call path, so the mock target in this test no longer matches. Please check the new call chain and update the mock accordingly.

Rebase

The branch currently has merge conflicts with main. Please rebase onto the latest main so we get a clean diff.

---

Once these are resolved, we will trigger a full panel evaluation of the PR. Happy to help if you have any questions -- just ping here. Thanks again!

[zeel2104]

Thanks for the follow-up. I addressed the remaining Ruff/CI and merge-conflict issues.

[sergio-sisternes-epam]

Hey @zeel2104 -- great progress on the strangler Stage 1 work! The Ruff threshold tightening and the mcp_integrator / publisher decompositions are solid. A few items remain before this fully satisfies #1077 -- details in the inline comments below.

Additionally, #1077 requires three files brought under 1400 lines: github_downloader.py (2316), commands/install.py (1680), and skill_integrator.py (1365). None of these have been trimmed in this PR, and commands/install.py appears to have grown. All three need to be addressed here -- this PR should fully close #1077 as stated in its "Fixes" line.

[github-actions]

🔍 APM Review Panel — PR #1113

Stage 1 strangler-fig refactoring: MCPIntegrator decomposition + threshold tightening

---

Panel Composition

Persona	Focus
🏗️ Architecture	Decomposition quality, cohesion, coupling
🔒 Correctness	Behavioral equivalence, edge cases
🧹 Code Quality	Typing, naming, testability
📐 API Design	Public/internal surface changes

---

What This PR Does

1. Tightens static-analysis thresholds — max-statements 275→200, max-branches 115→60, max-complexity 100→50, max-args 18→15, max-returns 18→12. These are real reductions, not cosmetic.
2. Decomposes MCPIntegrator.install() — extracts ~12 focused @staticmethod helpers (_normalise_user_scope, _split_mcp_dependencies, _discover_installed_runtimes, _select_target_runtimes, _runtimes_from_scripts, _filter_user_scope_runtimes, _install_registry_dependencies, _configure_registry_servers, _install_self_defined_dependencies, etc.).
3. Introduces Protocol types — _RuntimeManager, _MCPInstallLogger, _ConsolePrinter improve structural typing without coupling to concrete classes.
4. Adds InstallApmDependenciesOptions dataclass — cleans up _install_apm_dependencies() by replacing 14 explicit parameters with **kwargs + validated dataclass.

---

🏗️ Architecture — APPROVE

The decomposition follows SRP faithfully. Each extracted helper has a single, describable responsibility and a clear return type. The strangler-fig staging is explicit (# Stage 1 strangler threshold comments) and the approach is sound: tighten thresholds incrementally rather than big-bang rewriting.

The static-method pattern is appropriate here — these helpers carry no instance state and benefit from explicit dependency injection through their parameters.

Minor concern: _select_target_runtimes has a mild SRP violation: it both selects runtimes and emits user-facing progress logging. The side-effectful logging inside a selector makes it harder to test the selection logic in isolation. Consider returning only the selection result and letting the caller log, or passing a logger that can be a no-op in tests.

---

🔒 Correctness — APPROVE WITH NOTE

The decomposition appears behaviorally equivalent to the original monolith. Key logical flows are preserved:

• user_scope normalisation from InstallScope enum (now _normalise_user_scope)
• Project-scoped runtime gating (_gate_project_scoped_runtimes still called)
• Config drift detection delegated to existing _detect_mcp_config_drift
• Early-return / fallback paths (["vscode"] fallback on no installed runtimes)

One edge-case flag: In _filter_user_scope_runtimes, when all runtimes are filtered out the method logs a warning and returns []. The caller (install()) must then return 0 rather than continue. This contract is implicit — if the caller is ever refactored, a silent no-op could result. A raise or a richer return type (e.g., a Result) would make it explicit. Not a blocker, but worth tracking.

---

🧹 Code Quality — APPROVE WITH NOTE

Positive:

• Protocol classes are well-defined and appropriately narrow.
• InstallApmDependenciesOptions.from_kwargs() with unknown-key validation is clean defensive programming.
• _split_mcp_dependencies returning a 4-tuple is the one place where a named dataclass/NamedTuple would significantly improve readability at call sites — four positional return values are easy to unpack in the wrong order.
• noqa suppressions are applied surgically to the correct call sites.

Note on **kwargs in _install_apm_dependencies: This trades explicit IDE autocompletion at call sites for a cleaner signature. The from_kwargs guard preserves runtime safety, but static type checkers (mypy/pyright) will not validate call sites. This is an acceptable trade-off for a private/internal wrapper, but callers should be audited to confirm they all pass typed dicts or keyword arguments defensively.

---

📐 API Design — APPROVE

_install_apm_dependencies is an internal wrapper (underscore-prefixed), so the **kwargs API change is contained. MCPIntegrator.install() public signature is unchanged. InstallApmDependenciesOptions is a new exported name from request.py — the module is a good home for it.

---

CEO Arbitration

No panel conflicts requiring arbitration. All reviewers converge: the refactoring is sound, the behavioral equivalence is high-confidence, and the threshold reductions are genuine improvements. The notes above are refinements for follow-up PRs, not blockers.

---

📋 Advisory Recommendation

✅ APPROVE — with the following tracked follow-ups (non-blocking):

1. _split_mcp_dependencies return type → replace 4-tuple with a NamedTuple or small dataclass for safer unpacking.
2. _select_target_runtimes SRP → consider separating selection logic from progress logging to improve unit testability.
3. _filter_user_scope_runtimes implicit contract → document or enforce the "empty list ⇒ caller returns 0" contract at the call site.
4. Stage 2 ticket → _validate_package_exists (C901, PLR0911, PLR0915 suppressed) and run_mcp_install (PLR0913 suppressed) are the natural next targets for the strangler fig.

This PR is a clean, well-scoped refactoring. The threshold numbers are honest — they reflect actual code improvements, not just adjusted lint ceilings. Recommend merge.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• refactor: tighten stage 1 ruff thresholds #1113 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1113 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1113 · ● 601K · ◷

[zeel2104]

@sergio-sisternes-epam
Thanks for the detailed review. I addressed the remaining blockers in f2427d0:

• Trimmed the Stage 1 file-length outliers below 1400 lines and updated MAX_LINES=1400.
• Switched the marketplace publisher YAML write to yaml_io.dump_yaml().
• Refactored install/validation.py to meet the complexity target without the noqa waiver.
• Kept uv.lock out of the final changes.

Local verification:

• uv run --python 3.13 --extra dev ruff check src/ tests/
• architecture/file-length guardrail tests
• focused MCP/install/downloader regression tests

[sergio-sisternes-epam]

@zeel2104, main is moving fast in parallel to our refactor. Please fix the conflicts in github_downloader.py following the refactor @danielmeppiel introduced in #1223

[zeel2104]

@sergio-sisternes-epam

Thanks for the heads-up. I rebased the PR branch onto latest main and resolved the new conflicts in .github/workflows/ci.yml, commands/install.py, github_downloader.py, install/request.py, and install/validation.py.

For github_downloader.py, I kept the newer #1223 GitReferenceResolver/host-backend refactor and adjusted the Stage 1 file-length extraction around it rather than reverting that work.

[github-actions]

APM Review Panel: needs_rework

Stage 1 strangler refactor (#1077): 9 new focused modules, tightened complexity gates -- two blocking pre-merge fixes required (token leak in debug log, --frozen CLI regression).

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

The panel converges strongly on the structural value of this PR. Splitting github_downloader.py, install.py, skill_integrator.py, and marketplace/__init__.py into focused modules is a credible, well-scoped strangler fig Stage 1, and the threshold tightening (max-complexity 100->50, MAX_LINES 2450->1400) is a concrete, auditable commitment to complexity reduction. The python-architect and test-coverage-expert agree this is the right direction; the oss-growth-hacker correctly flags it as a contributor-credibility signal.

Two findings require author action before merge. First, auth-expert and supply-chain-security-expert agree independently: the _try_sparse_checkout debug path emits ' '.join(cmd) where cmd contains the raw auth_url with an embedded PAT/bearer token. Other error paths in this file already call _redact_token(); this path does not. Any user with APM_DEBUG set leaks their token to stderr. The fix is one line -- mask the auth URL before constructing the debug string. This is not a pre-existing gap introduced by the refactor; it is a regression introduced by moving the code path without carrying the redaction discipline forward. Second, the devx-ux-expert identifies that --frozen was deleted from the apm install CLI surface. The flag is documented as the recommended CI workflow pattern. The internal frozen field survives in InstallApmDependenciesOptions but no CLI flag populates it, so it silently defaults to False for all callers. CI pipelines following the published docs will silently stop enforcing lockfile freshness. Re-adding the Click option and the mutex guard is a small, targeted fix.

The nine recommended findings are real but not blocking. The most consequential post-merge items are: (1) the free-function-as-method pattern in github_downloader_packages.py creates a type-checker blind spot across the entire method body; (2) mcp_stale_cleanup.py bypasses the injected logger for 4 of 6 runtimes, breaking quiet-mode and test-capture contracts for copilot/codex/cursor/windsurf; (3) zero test files were added for 9 new modules and ~3000 lines of production code, leaving the module boundaries unguarded against silent drift. These three items collectively represent the highest regression-trap risk and should be the first follow-up PR after Stage 1 lands.

Dissent. supply-chain-security-expert classified the token-in-debug-log finding as 'nit'; auth-expert classified it as 'blocking'. The CEO sides with auth-expert: this is not a stylistic issue. Any user running APM with APM_DEBUG=1 against a private GitHub repo will have their PAT printed to stderr. The 'nit' classification likely reflects that the underlying pattern is pre-existing; however, the refactor moved the code without carrying the redaction discipline forward, making it a regression introduced by this PR.

Aligned with: Secure by Default (PARTIAL -- token redaction discipline exists in other paths but was not carried into the new _try_sparse_checkout debug path; must be fixed before merge). Governed by Policy (REGRESSION -- --frozen CLI flag deleted; lockfile enforcement silently disabled for all CI callers following published docs; must be restored before merge). Portable by Manifest (HOLD -- github_downloader_packages.py has zero direct tests; download and integrity behavior covered only transitively through mocks). Multi-Harness / Multi-Host (PARTIAL -- mcp_stale_cleanup.py correctly delegates for vscode and opencode but bypasses the logger for copilot/codex/cursor/windsurf). OSS Community Driven (OPPORTUNITY -- CHANGELOG and CONTRIBUTING both lack entries explaining the strangler pattern and new thresholds). Pragmatic as npm (POSITIVE -- splitting monolithic files into focused modules reduces cognitive load for first-time contributors).

Growth signal. The complexity threshold tightening (max-complexity 100->50, MAX_LINES 2450->1400) is the kind of durable, auditable quality signal that builds contributor trust over time. When Stage 2 ships, framing the cumulative reduction as a launch beat -- with before/after metrics -- will land well with the technically-minded OSS audience. A single CHANGELOG entry under [Unreleased] > Changed is the minimum; a short CONTRIBUTING section on the strangler pattern will reduce frustrated first-contributor issues materially.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	Solid strangler-fig Stage 1; free-function-as-method pattern in github_downloader_packages.py needs typed self; Protocol additions are a clear win.
CLI Logging Expert	0	1	1	mcp_stale_cleanup.py bypasses the logger for 4 of 6 runtimes, emitting _rich_success unconditionally even when a CommandLogger is present; all other new modules are clean.
DevX UX Expert	1	0	3	One blocking regression: --frozen flag removed from apm install CLI despite being documented as the recommended CI workflow; two nits on help text truncation.
Supply Chain Security Expert	0	0	1	Refactor preserves auth/path-safety invariants; one pre-existing token-in-debug-log risk carried into the new module warrants a nit.
OSS Growth Hacker	0	2	1	Refactor is a net positive for contributor onboarding; tightened thresholds are credibility signals. Add a CHANGELOG entry and a CONTRIBUTING note to amplify the growth surface.
Auth Expert	1	0	1	Token leak in _try_sparse_checkout debug log when 'git remote add' fails; auth-resolver chain in validation.py is preserved correctly.
Doc Writer	0	1	1	Pure internal refactoring; no user-facing behavior change. Two minor doc gaps: missing CHANGELOG entry and dev guide omits CI complexity-threshold enforcement pattern.
Test Coverage Expert	0	3	0	9 new modules, 0 test files added; mcp_stale_cleanup and github_downloader_packages carry the highest silent-drift risk on critical-promise surfaces.

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

Top 5 follow-ups

1. [Auth Expert] (blocking-severity) Mask auth_url before debug logging in _try_sparse_checkout when 'git remote add' fails -- Raw PAT/bearer token written to stderr for any user running APM_DEBUG=1 against a private repo. One-line fix; pattern already exists in adjacent code paths.
2. [DevX UX Expert] (blocking-severity) Restore --frozen Click option and mutex guard to apm install -- Flag is documented as the recommended CI workflow; deletion silently disables lockfile enforcement for all callers. Internal frozen field survives but is never populated from the CLI.
3. [CLI Logging Expert] Replace bare _rich_success() calls in mcp_stale_cleanup.py with logger.success() for all 6 runtimes -- Quiet-mode, test capture, and dry-run suppression currently work for vscode/opencode and silently break for copilot/codex/cursor/windsurf.
4. [Test Coverage Expert] Add integration-with-fixtures test for mcp_stale_cleanup.py exercising real IDE config file mutation -- Missing test on a multi-harness surface; transitive coverage through mocked facade does not catch file-mutation regressions.
5. [Test Coverage Expert] Add direct fixture-based tests for github_downloader_packages.py download and integrity paths -- 750-line module on a secure-by-default surface; existing tests mock at the boundary and miss behavioral changes inside the new module.

Architecture

classDiagram
    direction LR

    class GitHubPackageDownloader {
        <<Strategy>>
        +download_package()
        +download_subdirectory_package()
        +_try_sparse_checkout()
        +_get_clone_progress_callback()
    }
    class github_downloader_packages {
        <<Module>>
        +download_package(self)
        +download_subdirectory_package(self)
        +_try_sparse_checkout(self)
        +_get_clone_progress_callback(self)
    }
    class GitProgressReporter {
        <<ValueObject>>
        +update(op_code, cur_count, max_count)
    }
    class MCPIntegrator {
        <<Facade>>
        +configure()
        +remove_stale()
    }
    class _RuntimeManager {
        <<Protocol>>
        +is_runtime_available(runtime_name)
    }
    class _MCPInstallLogger {
        <<Protocol>>
        +progress(message)
        +warning(message)
        +error(message)
        +success(message)
    }
    class mcp_stale_cleanup {
        <<Module>>
        +remove_stale(stale_names, runtime, scope)
    }
    class SkillIntegrator {
        <<BaseIntegrator>>
        +integrate_skill()
    }
    class skill_helpers {
        <<Module>>
        +SkillIntegrationResult
        +integrate_skill_files()
    }

    GitHubPackageDownloader *-- github_downloader_packages : method injection
    github_downloader_packages ..> GitProgressReporter : uses
    MCPIntegrator ..> mcp_stale_cleanup : delegates remove_stale
    MCPIntegrator ..> _RuntimeManager : structural typing
    MCPIntegrator ..> _MCPInstallLogger : structural typing
    SkillIntegrator ..> skill_helpers : delegates

    class github_downloader_packages:::touched
    class GitProgressReporter:::touched
    class mcp_stale_cleanup:::touched
    class skill_helpers:::touched
    class _RuntimeManager:::touched
    class _MCPInstallLogger:::touched

    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A([apm install]) --> B[commands/install.py]
    B --> C[install/mcp/handler.py]
    C --> D[integration/mcp_integrator.py MCPIntegrator.configure]
    D --> E[FS: write runtime config files]
    B --> F[install/manifest_update.py]
    F --> G[FS: write apm.lock.yaml]
    B --> H[install/rollback.py]
    H --> I[FS: restore prior state on failure]

    J([apm install stale cleanup]) --> K[mcp_integrator.py MCPIntegrator.remove_stale]
    K --> L[mcp_stale_cleanup.py remove_stale]
    L --> M{runtime loop}
    M --> N[vscode mcp.json]
    M --> O[copilot mcp-config.json]
    M --> P[codex config.toml]
    M --> Q[cursor mcp.json]
    M --> R[opencode.json]
    M --> S[windsurf mcp_config.json]

    T([apm install github dep]) --> U[deps/github_downloader.py]
    U --> V[deps/github_downloader_packages.py method injected]
    V --> W{sparse checkout?}
    W -- yes --> X[git sparse-checkout]
    W -- no --> Y[git clone via GitPython]
    V --> Z[deps/github_progress.py GitProgressReporter]

Loading

Recommendation

Fix the two blocking items -- token redaction in _try_sparse_checkout and the --frozen CLI regression -- before merge. Both are small, targeted changes. Once those land, the seven recommended follow-ups are best tracked as a Stage 1.5 PR: logger consistency in mcp_stale_cleanup, integration tests for mcp_stale_cleanup and github_downloader_packages, typed self for the free-function-as-method pattern, and CHANGELOG/CONTRIBUTING entries. The structural direction is sound and worth shipping quickly after the two blocking fixes are in.

---

Full per-persona findings

Python Architect

• [recommended] Free functions in github_downloader_packages.py take untyped self -- IDE and type-checker blind spot at src/apm_cli/deps/github_downloader_packages.py:25
  Functions _try_sparse_checkout, download_subdirectory_package, download_package, _get_clone_progress_callback are module-level callables that accept self without a type annotation, then are assigned as class attributes on GitHubPackageDownloader. mypy/pyright cannot reason about self.* accesses, so the entire method body is type-opaque. This cross-file method injection is the only instance of the pattern in the codebase and creates a maintenance surprise.
  Suggested: Add a TYPE_CHECKING-guarded Protocol or forward-ref: if TYPE_CHECKING: from .github_downloader import GitHubPackageDownloader and annotate def _try_sparse_checkout(self: GitHubPackageDownloader, ...) -> bool:

• [nit] RUF013 suppressed with noqa instead of fixed in mcp_stale_cleanup.py at src/apm_cli/integration/mcp_stale_cleanup.py:17
runtime: str = None # noqa: RUF013 and exclude: str = None # noqa: RUF013 suppress the implicit-Optional lint rather than changing the annotation to str | None = None.
  Suggested: runtime: str | None = None, exclude: str | None = None,

• [nit] remove_stale still carries C901/PLR0912 noqa after extraction -- complexity not reduced, only relocated at src/apm_cli/integration/mcp_stale_cleanup.py:15
  Moving from mcp_integrator.py preserved the per-runtime if-block structure. Stage 1 goal is file-size reduction so this is expected, but a follow-up to extract a _clean_runtime_config(runtime, config_path, key, stale, logger) helper would eliminate the noqa.

CLI Logging Expert

• [recommended] mcp_stale_cleanup.py calls _rich_success() directly for copilot/codex/cursor/windsurf runtimes, bypassing the injected logger at src/apm_cli/integration/mcp_stale_cleanup.py
  remove_stale() replaces a None logger with NullCommandLogger so logger is guaranteed non-None. Yet copilot, codex, cursor, and windsurf stale-removal blocks call _rich_success() unconditionally instead of logger.success(). vscode and opencode correctly call logger.progress(). The inconsistency means quiet-mode, test capture, and dry-run suppression work for two runtimes and silently break for four others.
  Suggested: Replace every bare _rich_success(...) call in remove_stale() with logger.success(...). Remove the _rich_success import once that is done.

• [nit] mcp_stale_cleanup.py uses logger.progress() for removal confirmations; logger.success() is the correct semantic at src/apm_cli/integration/mcp_stale_cleanup.py
  progress() maps to blue [>] (in-flight activity). Removal of a stale server is a completed action and should be logger.success() so it renders green [+] and matches the traffic-light rule.

DevX UX Expert

• [blocking] --frozen flag silently removed from apm install, breaking documented CI workflow at src/apm_cli/commands/install.py
  The --frozen flag is documented as the recommended CI pattern ('apm install --frozen'). The flag and its mutex guard (--frozen + --update raise UsageError) were deleted from the install command. The internal frozen field still exists in InstallApmDependenciesOptions (frozen=options.frozen called at line 1392) but no CLI flag populates it -- it always defaults to False. Any CI pipeline following the published docs will silently stop enforcing lockfile freshness.
  Suggested: Re-add @click.option("--frozen", is_flag=True, ...) to the install command, restore the frozen parameter in the function signature, and restore the mutex guard if frozen and update: raise click.UsageError(...).
  Proof (missing): tests/integration/test_install_frozen.py -- proves: apm install --frozen rejects installs when lockfile is missing or stale, matching documented CI contract [devx,governed-by-policy]

• [nit] --update help text lost deprecation guidance pointing users to 'apm update' at src/apm_cli/commands/install.py
  Previous text included '(deprecated: prefer apm update for an interactive plan)'. New text drops this, reducing discoverability.
  Suggested: Restore: help="Update dependencies to latest Git references (deprecated: prefer 'apm update' for an interactive plan, or 'apm update --yes' for CI)"

• [nit] --force help text truncated, dropping 'does NOT refresh refs; use apm update for that' guidance at src/apm_cli/commands/install.py
  The old help text explicitly told users that --force does not refresh refs. Dropping this increases the chance a user mistakenly uses --force when they need --update.
  Suggested: Restore: help="Overwrite locally-authored files on collision and deploy despite critical security findings (does NOT refresh refs; use 'apm update' for that)"

• [nit] compile() still carries # noqa: C901, PLR0912, PLR0915 suppressions post-refactor at src/apm_cli/commands/compile/cli.py:46
  The function still exceeds Stage 1 strangler targets and will need its own dedicated split PR.
  Suggested: Open a follow-up issue to split compile() the same way install.py was split.

Supply Chain Security Expert

• [nit] Raw auth_url (with embedded token) can appear in debug log when 'git remote add origin' fails in _try_sparse_checkout at src/apm_cli/deps/github_downloader_packages.py
  The cmd list contains auth_url verbatim. If that step fails, ' '.join(cmd) is passed to _compat._debug, leaking the token into debug-mode log. Other error paths in the same file consistently call _redact_token() -- this one does not.
  Suggested: Replace ' '.join(cmd) with ' '.join(_redact_token(c) for c in cmd) or mask the auth_url before inserting into cmds.

OSS Growth Hacker

• [recommended] No CHANGELOG entry for Stage 1 strangler refactor leaves the OSS community unaware of the structural improvement at CHANGELOG.md
  Contributors scanning CHANGELOG to understand codebase trajectory miss a clear signal that APM is actively reducing complexity debt.
  Suggested: Add under [Unreleased] > Changed: 'Stage 1 strangler refactor (Strangler Stage 1: tame the worst outliers #1077): split github_downloader.py, commands/install.py, skill_integrator.py, and marketplace/init.py into focused modules; complexity thresholds tightened (max-complexity 100->50, max-branches 115->60, MAX_LINES 2450->1400).'

• [recommended] No contributor-facing documentation of the strangler pattern or complexity-gate rationale at CONTRIBUTING.md
  First-time contributors hitting CI failure on max-complexity 50 with no guidance will open frustrated issues or abandon PRs.
  Suggested: Add a 'Module complexity policy' section explaining the strangler fig pattern, pointing to issue Strangler Stage 1: tame the worst outliers #1077, listing current thresholds, and giving a one-command example.

• [nit] CI comment 'Stage 1 strangler target (Strangler Stage 1: tame the worst outliers #1077)' is internal jargon in CI output at .github/workflows/ci.yml
  External contributors reading CI output won't know what it means. Replace with 'complexity ceiling (see CONTRIBUTING.md#module-complexity-policy)'.

Auth Expert

• [blocking] Token embedded in auth_url leaks to stderr via _debug when 'git remote add' fails at src/apm_cli/deps/github_downloader_packages.py:82
  In _try_sparse_checkout, auth_url contains the PAT/bearer token inline ((token/redacted)@host/...). The failure debug path does ' '.join(cmd) on the full cmd list, which includes auth_url at index 3. Any user with APM_DEBUG set will have their token printed to stderr. Tokens in URLs must be masked before logging.
  Suggested: Build a parallel 'safe_cmds' list where the 'git remote add origin url' entry has the token redacted. Pattern already used elsewhere: replace '(token/redacted)@' with 'https://***@'.

• [nit] dep_auth_scheme defaults to 'basic' when dep_auth_ctx is None, silently hiding a missing AuthContext at src/apm_cli/deps/github_downloader_packages.py:43
  If _resolve_dep_auth_ctx returns None, dep_auth_scheme silently falls back to 'basic' and _build_repo_url is called with a None token. Same behavior as before the split but harder to trace in the new module.

Doc Writer

• [recommended] development-guide.md does not document the CI module-size and complexity threshold enforcement pattern at docs/src/content/docs/contributing/development-guide.md
  Contributors hitting CI failures on max-complexity 50 with no documented explanation. Development guide covers Ruff but not the enforced thresholds or ratchet strategy.
  Suggested: Add a 'Complexity and module-size limits' subsection: explain the strangler fig pattern, note the thresholds live in pyproject.toml and ci.yml, state that thresholds are ratcheted down in dedicated refactoring PRs.

• [nit] CHANGELOG.md has no entry for the Stage 1 strangler refactoring at CHANGELOG.md
  The [Unreleased] section omits this structural refactoring. Keep a Changelog recommends recording changes that affect contributors.
  Suggested: Under [Unreleased] > Changed: 'Stage 1 strangler-fig refactoring splits mcp_integrator.py, commands/install.py, and related monoliths into focused modules; CI complexity ceiling lowered from 100 to 50 (issue Strangler Stage 1: tame the worst outliers #1077).'

Test Coverage Expert

• [recommended] mcp_stale_cleanup.py (329 lines) has no direct test at integration-with-fixtures tier at src/apm_cli/integration/mcp_stale_cleanup.py
  Existing tests call MCPIntegrator.remove_stale() with mocked config paths -- they verify the facade delegates but do not exercise the file-mutation logic against real IDE config fixtures. Grep of tests/ for mcp_stale_cleanup produced no integration-tier hit.
  Suggested: Add a test writing a real Claude/Copilot config file with stale entries, call MCPIntegrator.remove_stale(), assert entries absent from disk.
  Proof (missing at integration-with-fixtures): tests/integration/test_mcp_stale_cleanup_integration.py::test_remove_stale_entries_from_claude_config -- proves: apm install removes stale MCP server entries from target IDE configs when a package is replaced or removed [multi-harness-support,devx]
  assert 'old-pkg' not in json.loads(config_path.read_text())['mcpServers']

• [recommended] github_downloader_packages.py (750 lines) is covered only transitively; unit tests mock git.Repo before the new code runs at src/apm_cli/deps/github_downloader_packages.py
  tests/test_github_downloader.py mocks at the boundary so a subtle behavioral change inside github_downloader_packages would not be caught. Grep of tests/ for github_downloader_packages returned zero hits.
  Suggested: Add a fixture-based test that stubs a local git repo and asserts download + integrity behavior without network mocking.
  Proof (missing at integration-with-fixtures): tests/integration/test_github_downloader_packages_integration.py::test_download_package_from_local_git_fixture -- proves: apm install downloads and extracts a GitHub package without corrupting or silently skipping files [portability-by-manifest,secure-by-default]
  assert (dest / 'apm.yml').exists()

• [recommended] Zero test files added for a 3000-line, 9-new-module refactor; no regression-trap guards the new module boundaries at src/apm_cli/install/manifest_update.py
  No test file appears in the diff (confirmed via diff inspection). While import chains are covered transitively, none of the 9 new modules has a test that explicitly imports and exercises the module's public API. A subtle signature change or missing re-export in any of these modules would only surface at runtime for users.
  Suggested: Add minimal unit tests per new module that import the primary public symbol and call it with a fixture.
  Proof (missing at unit): tests/unit/install/test_manifest_update.py::test_check_package_conflicts_empty -- proves: The manifest update module is importable and its core conflict-detection logic behaves correctly after extraction from commands/install.py [portability-by-manifest,devx]
  assert _check_package_conflicts([]) == set()

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

Note

🔒 Integrity filter blocked 7 items

The following items were blocked because they don't meet the GitHub integrity level.

• #1113 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• refactor: tighten stage 1 ruff thresholds #1113 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1113 search_pull_requests: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• b6fa2de list_commits: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• 0bd846f list_commits: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• 985118f list_commits: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• cb07ab0 list_commits: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1113 · ● 7.4M · ◷

[sergio-sisternes-epam]

@zeel2104 looks like we got another (small) conflict, and we've got two recommendations from the APM Panel.

Let's get these two over the line and merge them. Thanks for the continued effort!

[zeel2104]

@sergio-sisternes-epam
Fixed and pushed.

Changes:

• resolved the latest conflict by rebasing onto current main
• masked the auth URL before _try_sparse_checkout debug logging
• restored the apm install --frozen CLI option, mutex guard, and request wiring
• kept the Stage 1 1400-line guardrail clean after the rebase