refactor: tighten stage 1 ruff thresholds#1113
Conversation
zeel2104
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
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
**kwargsvalidation.
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). |
|
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 There are still a few CI blockers that need addressing before we can proceed: CI failures
RebaseThe branch currently has merge conflicts with 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! |
|
Thanks for the follow-up. I addressed the remaining Ruff/CI and merge-conflict issues. |
sergio-sisternes-epam
added
the
panel-review
sergio-sisternes-epam
left a comment
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
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.
sergio-sisternes-epam
added
panel-review
🔍 APM Review Panel — PR #1113
Panel Composition
What This PR Does
🏗️ Architecture — APPROVEThe decomposition follows SRP faithfully. Each extracted helper has a single, describable responsibility and a clear return type. The strangler-fig staging is explicit ( The static-method pattern is appropriate here — these helpers carry no instance state and benefit from explicit dependency injection through their parameters. Minor concern: 🔒 Correctness — APPROVE WITH NOTEThe decomposition appears behaviorally equivalent to the original monolith. Key logical flows are preserved:
One edge-case flag: In 🧹 Code Quality — APPROVE WITH NOTEPositive:
Note on 📐 API Design — APPROVE
CEO ArbitrationNo 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):
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 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
|
@sergio-sisternes-epam
Local verification:
|
zeel2104
force-pushed
the
strangler-stage-1-ruff-thresholds
branch
from
f2427d0 to
0c178e0
Compare
|
@zeel2104, main is moving fast in parallel to our refactor. Please fix the conflicts in |
Head branch was pushed to by a user without write access
zeel2104
force-pushed
the
strangler-stage-1-ruff-thresholds
branch
from
2ae81dc to
b6fa2de
Compare
|
Thanks for the heads-up. I rebased the PR branch onto latest main and resolved the new conflicts in For |
sergio-sisternes-epam
removed
the
panel-review
sergio-sisternes-epam
added
the
panel-review
APM Review Panel:
|
| 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
- [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.
- [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.
- [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.
- [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.
- [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
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]
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 atsrc/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 acceptselfwithout a type annotation, then are assigned as class attributes on GitHubPackageDownloader. mypy/pyright cannot reason aboutself.*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 GitHubPackageDownloaderand annotatedef _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: RUF013andexclude: str = None # noqa: RUF013suppress the implicit-Optional lint rather than changing the annotation tostr | 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() withlogger.success(...). Remove the_rich_successimport 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 guardif 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 | noneGenerated by PR Review Panel for issue #1113 · ● 7.4M · ◷
github-actions
Bot
removed
the
panel-review
Description
Tightens the Stage 1 Ruff complexity thresholds for the strangler-fig refactor work and trims the main outliers blocking those guardrails.
Changes include:
MCPIntegrator.installby extracting runtime selection, registry install, self-defined install, and summary helpers.pyproject.tomlto the Stage 1 targets.Fixes #1077
Type of change
Testing
Validation run: