Skip to content

doctor: _is_plugin_installed spawns 'claude plugin list' 3x per run (production latency) #4814

Description

@Trecek

Problem (production latency — explicitly NOT a CI-time fix)

_is_plugin_installed() (src/autoskillit/cli/_init_helpers.py:338) spawns a real
subprocess.run(["claude", "plugin", "list"], ..., timeout=10) and is called three separate times, uncached, within a single _collect_doctor_results():

  • _check_hook_registration_doctor_hooks.py:19 (import :20, call :22)
  • _check_hook_registry_drift_doctor_hooks.py:59 (import :76, call :78)
  • _check_dual_registration_doctor_hooks.py:152 (import :155, call :157) — note this one uses the negated form if not _is_plugin_installed():

Correction to an earlier draft: the three call sites were previously mislabelled (the :76 and :155 sites were swapped), and a supposed fourth call in _doctor_mcp.py was cited — that file contains zero references to _is_plugin_installed. The complete set of src/ call sites is five: the three above plus cli/update/_update_checks.py:118 and cli/_hooks.py:128, neither of which is on the doctor path. The in-scope collapse is 3 → 1. Re-derive any total-spawn figure from an actual subprocess spy rather than trusting the earlier "4 → 2" framing. Roughly ~1.3s of user-facing latency per autoskillit doctor invocation.

⚠️ The drift check is reached through a wrapper, not directly

_collect_doctor_results() does not call _check_hook_registry_drift directly — it calls
_check_hook_registry_drift_all_scopes(Path.cwd()) (doctor/__init__.py:142; wrapper defined at
_doctor_hooks.py:120), which loops over up to three scopes (user/project/local) and invokes the
raw check per scope. plugin_installed must be threaded through that wrapper too, otherwise
this check keeps re-resolving — and note it may currently spawn the subprocess once per scope.

The team already treats this function as expensive: tests/arch/test_ast_rules.py:1865 carries a guard banning _is_plugin_installed from the session launch path, documented as "_is_plugin_installed runs 'claude plugin list' as a subprocess (up to 10s)." This fix is aligned with that intent. (Two footnotes: that guard AST-scans only _session_launch.py/_session_cook.py, so it does not constrain doctor/; and the "ARCH-009" label in that comment is reused — a different, real rule_id ARCH-009 exists elsewhere in the same file for logger naming. Pre-existing source landmine, not introduced here.)

⚠️ This delivers NO CI speedup — do not expect one

tests/cli/conftest.py:38 is an autouse=True fixture that at line 49 does:

monkeypatch.setattr("autoskillit.cli._init_helpers._is_plugin_installed", lambda **kwargs: False)

Every test in tests/cli/ has this function mocked; it never spawns a subprocess there. Other files monkeypatch it too (tests/contracts/test_hook_path_relocatability.py:75, tests/hooks/test_hook_executability.py:143,197, tests/cli/test_hook_drift_plugin_guard.py:23,37).

Measured on tests/cli/test_doctor.py: median A/B ratio 0.997 — no measurable change. An earlier estimate of 8–20s off the CI shard was extrapolated from a standalone profile that bypassed the suite's own mocking discipline; that estimate is retracted.

File this for the production win only.

Fix

Resolve _is_plugin_installed(...) once at the top of _collect_doctor_results() — alongside the existing _backend resolution (src/autoskillit/cli/doctor/__init__.py:97-105) — and thread the resulting bool into the three checks.

_backend is threaded via lambda closures for checks that take arguments (e.g. _run_check(lambda: _check_stale_mcp_servers(..., backend=_backend), ...)). The functools.partial calls at __init__.py:121-127 are for zero-argument checks only. Follow the lambda pattern, since these three checks take arguments.

Give the three _check_* signatures a plugin_installed: bool | None = None default so existing direct-call unit tests keep working unchanged.

⚠️ None must mean "resolve it yourself", not "False"

None is falsy, so a naive guard silently takes the not-installed branch:

if plugin_installed:          # WRONG — None falls through as "not installed"

Each check must explicitly fall back:

if plugin_installed is None:
    from autoskillit.cli._init_helpers import _is_plugin_installed   # keep LOCAL
    plugin_installed = _is_plugin_installed()

Note _check_dual_registration uses the negated form (if not _is_plugin_installed():) — re-check that branch's semantics after threading the bool.

tests/cli/test_doctor.py's DC-14/DC-15/DC-16 call these checks directly with no new kwarg, relying on the monkeypatched _is_plugin_installed. The None fallback above is what keeps them working — without it they silently take the wrong branch and the monkeypatch is never consulted.

⚠️ Implementation trap — keep the import LOCAL

_doctor_hooks.py imports _is_plugin_installed inside each function body (lines 20, 76, 155), never at module level. That locality is load-bearing, not stylistic — it is what makes tests/cli/conftest.py's monkeypatch.setattr("autoskillit.cli._init_helpers._is_plugin_installed", ...) effective.

Hoisting the import to module level in doctor/__init__.py silently defeats that monkeypatch (a top-level from X import Y binds a stale reference at import time that monkeypatch.setattr on X's own module never rebinds) and breaks test_doctor_ignores_healthy_coregistered_servers. This was hit and fixed during prototyping. Keep the import inside _collect_doctor_results().

Acceptance criteria

  • claude plugin list is spawned once per _collect_doctor_results() (assert via call counter or subprocess spy), down from 3 in-scope calls.
  • Full tests/cli/ passes — the prototype measured 1,895 passed, 72 skipped, 0 failed.
  • test_doctor_ignores_healthy_coregistered_servers passes (the specific test the import-hoist trap breaks).
  • Existing direct-call unit tests of the three _check_* functions pass unchanged (hence the None default).
  • No @functools.lru_cache is introduced on _is_plugin_installed — a bare cache breaks tests/cli/test_init_helpers.py::test_claude_code_backend_calls_subprocess, which asserts subprocess.run is called exactly once (xdist workers are long-lived, so a poisoned global cache yields 0).

Expected impact

~1.3s off every real autoskillit doctor run. Zero CI-time change — that is expected, not a regression.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions