From b38f47c15485428478be1c9b28e76d718d417cf7 Mon Sep 17 00:00:00 2001 From: Kasper Junge Date: Tue, 9 Jun 2026 14:21:37 +0200 Subject: [PATCH 1/2] docs: add TASK.md for radon complexity-reduction work Defines the complexity-reduction task and acceptance criteria so the refactor iterations have a shared target to build toward. Co-Authored-By: Claude Opus 4.8 --- .../TASK.md | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 tasks/2026-06-09-radon-complexity-reduction/TASK.md diff --git a/tasks/2026-06-09-radon-complexity-reduction/TASK.md b/tasks/2026-06-09-radon-complexity-reduction/TASK.md new file mode 100644 index 0000000..1a4256f --- /dev/null +++ b/tasks/2026-06-09-radon-complexity-reduction/TASK.md @@ -0,0 +1,60 @@ +## Task + +Reduce cyclomatic complexity and improve maintainability in the `agr` codebase's hotspot functions, guided by radon metrics, and add a CI gate (xenon) to prevent regressions. The work is decomposition/refactor only — no behavior changes. + +## Relevant Codebase + +Radon baseline (run via `uvx radon cc agr/ agrx/ -a -s` and `uvx radon mi`): +- **Overall**: 419 blocks, avg CC **A (3.9)**; every file MI grade **A**. Healthy baseline. +- **Worst functions**: + - `run_remove` — `agr/commands/remove.py:209` — **CC 36 (E)**, the single biggest outlier. + - `_install_package` — `agr/commands/add.py:225` — **CC 21 (D)**. + - `detect_conflicts` — `agr/package.py:229` — CC 19 (C); `expand_packages` `agr/package.py:127` — CC 15 (C). + - `_run_install_pipeline` — `agr/commands/sync.py:748` — CC 17 (C), plus six more C-grade functions in the same file. + - `_parse_dependencies_from_doc` `agr/config.py:346` (15) and `AgrConfig.save` `agr/config.py:558` (14). + - `run_config_add` `agr/commands/config_cmd.py:281` (15); `run_list` `agr/commands/list.py:65` (14); `run_add` `agr/commands/add.py:391` (14); `_migrate_skills_directory` `agr/commands/migrations.py:48` (15). +- **Lowest MI files**: `agr/commands/sync.py` 19.3, `agr/commands/config_cmd.py` 29.7, `agr/config.py` 29.9, `agr/commands/add.py` 37.9. +- **Largest file**: `agr/commands/sync.py` — **946 SLOC**, ~2× the next largest. + +How it works / patterns to follow: +- Commands live under `agr/commands/`, each exposing a `run_*` orchestrator that delegates to private helpers and returns a `CommandResult` (`agr/commands/__init__.py`). `sync.py` already models good decomposition: `_classify_dependencies` → `_run_install_pipeline` with `_ClassifiedDeps`/`SyncResult` dataclasses. New extractions should mirror this seam-based style. +- `run_remove` already has helpers available to lean on: `_transitive_leaf_entries_for_packages`, `_nested_package_entries_for_packages`, `_find_dep_by_candidates`, `_uninstall_from_filesystem`, `_update_lockfile_after_remove`. +- Per `agr/CLAUDE.md`: agr and agrx stay unified; add/remove should remain symmetric; include tests for what's implemented; tooling is `uv run` (pytest, ruff, ty) and `mkdocs build --strict` for docs. + +Integration points: config parsing/serialization round-trips through `agr/config.py` (TOML, comment/order preservation); `package.py` conflict logic runs on every add/sync (high blast radius). + +## Goal + +The hotspot functions are decomposed into smaller, individually testable units with no D/E-grade functions remaining (target: no function above CC 10, i.e. grade ≤ B), `sync.py` is split into focused modules, and CI fails if new code reintroduces high complexity — all with existing behavior unchanged. + +## Acceptance Criteria + +1. `run_remove` (`agr/commands/remove.py`) is refactored to CC ≤ 10, splitting resolution → filesystem-uninstall → lockfile-update phases, with a per-branch test matrix (local/remote, package/leaf, transitive cleanup). +2. `_install_package` (`agr/commands/add.py`) is refactored to CC ≤ 10, keeping add/remove handling symmetric; tests cover local-vs-remote and conflict/duplicate branches. +3. `agr/commands/sync.py` is split into focused modules (e.g. classify / pipeline / lockfile) with `run_sync` as a thin entry point; the largest resulting file is materially smaller than 946 SLOC and its MI grade improves from the 19.3 baseline. +4. `detect_conflicts` and `expand_packages` (`agr/package.py`) are each refactored to CC ≤ 10. +5. The config parser/serializer hotspots (`_parse_dependencies_from_doc`, `AgrConfig.save` in `agr/config.py`) are refactored to CC ≤ 10, with golden-file round-trip tests asserting parse→save→parse identity (including comment/order preservation). +6. Remaining C-grade command functions (`run_config_add`, `run_list`, `run_add`, `_migrate_skills_directory`) are reduced to grade ≤ B. +7. A xenon complexity gate runs in CI alongside ruff/ty (e.g. `uvx xenon --max-absolute B --max-modules B --max-average A agr agrx`), tightened to its final thresholds only after criteria 1–6 land. +8. `uv run pytest`, `uv run ruff check .`, and `uv run ty check` all pass; no observable CLI behavior changes for `agr`/`agrx`. + +## Scope + +### In scope +- Refactoring the named hotspot functions and splitting `sync.py`. +- Adding/expanding unit tests for the refactored code paths and config round-trips. +- Adding the xenon CI gate. + +### Out of scope +- Any change to CLI behavior, command surface, or `agr.toml`/`agr.lock` formats. +- Raising comment density / docstring coverage (4% C%L baseline is acceptable for this idiom). +- Refactoring functions already at grade ≤ B that aren't named above. +- New features. + +## Risks + +- **`run_remove` (CC 36)** has many interacting branches (transitive/nested package cleanup, lockfile sync); easy to drop an edge case — lean on existing helpers and lock behavior with tests before refactoring. +- **`sync.py` split** is the highest-blast-radius structural change; import cycles and the lockfile-build path are the fragile spots. Do it after the smaller wins, on a clear runway. +- **Config round-trip** refactors risk breaking TOML comment/ordering preservation — golden-file identity tests are the safety net. +- **`package.py`** conflict logic runs on every add/sync; regressions are high-impact. +- xenon thresholds must be staged (start lenient, tighten last) or CI will be red mid-effort. From b1e3a82d941536437e18b331a3d34539319ac6b0 Mon Sep 17 00:00:00 2001 From: Kasper Junge Date: Tue, 9 Jun 2026 14:21:47 +0200 Subject: [PATCH 2/2] refactor: decompose run_remove so per-ref logic is individually testable run_remove was the worst complexity outlier (radon CC 36, grade E). Its per-ref loop body is extracted into focused helpers (_resolve_dep, _remove_from_filesystem, the _resolve_package_cleanup trio, _process_ref, and a _RefRemoval result struct), leaving run_remove a thin loop. No behavior change; all functions now grade <= B (run_remove itself A 2). Adds seam-level tests for package child resolution and per-ref processing, plus a LOG.md and implementation diary for the iteration. Co-Authored-By: Claude Opus 4.8 --- agr/commands/remove.py | 362 ++++++++++++------ .../2026-06-09-radon-complexity-reduction.md | 113 ++++++ .../LOG.md | 37 ++ tests/unit/test_remove.py | 139 +++++++ 4 files changed, 544 insertions(+), 107 deletions(-) create mode 100644 docs/diary/2026-06-09-radon-complexity-reduction.md create mode 100644 tasks/2026-06-09-radon-complexity-reduction/LOG.md diff --git a/agr/commands/remove.py b/agr/commands/remove.py index baab8d8..023eb0a 100644 --- a/agr/commands/remove.py +++ b/agr/commands/remove.py @@ -2,6 +2,7 @@ from __future__ import annotations +from dataclasses import dataclass, field from pathlib import Path from agr.commands import CommandResult @@ -10,6 +11,7 @@ DEPENDENCY_TYPE_PACKAGE, DEPENDENCY_TYPE_RALPH, DEPENDENCY_TYPE_SKILL, + AgrConfig, Dependency, ) from agr.commands.migrations import run_tool_migrations @@ -206,6 +208,247 @@ def _find_dep_by_candidates( return None +@dataclass +class _RefRemoval: + """Outcome of processing a single ref in ``run_remove``. + + ``removed_candidates`` / ``removed_kinds`` are parallel lists describing + every lockfile entry that should be dropped as a result of this ref + (the dep itself plus any transitive/nested children), in the same shape + ``_update_lockfile_after_remove`` consumes. + """ + + result: CommandResult + removed_candidates: list[list[str]] = field(default_factory=list) + removed_kinds: list[str] = field(default_factory=list) + + +def _nested_packages_to_remove( + dep: Dependency, + existing_lockfile: Lockfile | None, +) -> list[tuple[str, LockedEntry]]: + """Nested package entries whose whole parent chain lives inside ``dep``.""" + candidate_package_ids = ( + existing_lockfile.package_closure({dep.identifier}) + if existing_lockfile + else {dep.identifier} + ) + return [ + (kind, entry) + for kind, entry in _nested_package_entries_for_packages( + existing_lockfile, {dep.identifier} + ) + if not (entry.parent_ids - candidate_package_ids) + ] + + +def _transitive_leaves_to_remove( + dep: Dependency, + config: AgrConfig, + existing_lockfile: Lockfile | None, + nested_package_entries: list[tuple[str, LockedEntry]], +) -> list[tuple[str, LockedEntry]]: + """Transitive leaf entries to remove, excluding ones still directly required. + + A leaf is removed only when it is not a direct dependency and its entire + parent chain lives inside the package being removed (plus the nested + packages also being removed). + """ + direct_leaf_ids = { + (d.type, d.identifier) + for d in config.dependencies + if not d.is_package and d.identifier != dep.identifier + } + removed_package_ids = {dep.identifier} | { + entry.identifier for _kind, entry in nested_package_entries + } + return [ + (kind, entry) + for kind, entry in _transitive_leaf_entries_for_packages( + existing_lockfile, {dep.identifier} + ) + if (kind, entry.identifier) not in direct_leaf_ids + and not (entry.parent_ids - removed_package_ids) + ] + + +def _resolve_package_cleanup( + dep: Dependency, + config: AgrConfig, + existing_lockfile: Lockfile | None, +) -> tuple[list[tuple[str, LockedEntry]], list[tuple[str, LockedEntry]]]: + """Resolve the nested-package and transitive-leaf entries to remove for a package. + + Returns ``(nested_package_entries, transitive_entries)``. A child is + only scheduled for removal when its entire parent chain lives inside the + package being removed (so children still required by another package are + retained). + """ + nested_package_entries = _nested_packages_to_remove(dep, existing_lockfile) + transitive_entries = _transitive_leaves_to_remove( + dep, config, existing_lockfile, nested_package_entries + ) + return nested_package_entries, transitive_entries + + +def _uninstall_transitive_entries( + transitive_entries: list[tuple[str, LockedEntry]], + config: AgrConfig, + tools: list[ToolConfig], + repo_root: Path | None, + skills_dirs: dict[str, Path] | None, +) -> bool: + """Filesystem-remove each transitive child entry. Returns True if any removed.""" + removed = False + for kind, entry in transitive_entries: + child_handle = _entry_to_handle(entry, kind, config.default_owner) + if _uninstall_from_filesystem( + child_handle, + kind == DEPENDENCY_TYPE_RALPH, + tools, + repo_root, + entry.source, + skills_dirs, + default_repo=config.default_repo, + ): + removed = True + return removed + + +def _resolve_dep( + ref: str, + config: AgrConfig, + global_install: bool, +) -> tuple[ParsedHandle, list[str], Dependency | None]: + """Resolve a ref to its handle, identifier candidates, and matching dependency.""" + handle = parse_handle(ref, default_owner=config.default_owner) + + # Compute the resolved absolute path once for local global installs. + abs_path_str: str | None = None + if global_install and handle.is_local and handle.local_path is not None: + abs_path_str = str(handle.resolve_local_path()) + + candidates = _identifier_candidates(ref, handle, abs_path_str) + dep = _find_dep_by_candidates(candidates, ref, config.dependencies) + + # When the dep was found by installed_name fallback (not by any candidate + # identifier), its actual identifier must be added so that config and + # lockfile removal can match it. + if dep is not None and dep.identifier not in candidates: + candidates.append(dep.identifier) + + return handle, candidates, dep + + +def _remove_leaf_from_filesystem( + dep: Dependency | None, + handle: ParsedHandle, + config: AgrConfig, + tools: list[ToolConfig], + repo_root: Path | None, + skills_dirs: dict[str, Path] | None, +) -> bool: + """Filesystem-remove a non-package dependency (skill or ralph).""" + source_name = None + if dep and dep.is_remote: + source_name = dep.source or config.default_source + is_ralph = dep is not None and dep.is_ralph + fs_handle = dep.to_parsed_handle(config.default_owner) if dep else handle + return _uninstall_from_filesystem( + fs_handle, + is_ralph, + tools, + repo_root, + source_name, + skills_dirs, + default_repo=config.default_repo, + ) + + +def _remove_from_filesystem( + dep: Dependency | None, + handle: ParsedHandle, + config: AgrConfig, + tools: list[ToolConfig], + repo_root: Path | None, + skills_dirs: dict[str, Path] | None, + existing_lockfile: Lockfile | None, +) -> tuple[bool, list[tuple[str, LockedEntry]], list[tuple[str, LockedEntry]]]: + """Filesystem-remove a dependency, fanning out to package children when needed. + + Returns ``(removed_anything, nested_package_entries, transitive_entries)``. + Packages have no filesystem footprint of their own; only their transitive + children are removed. + """ + if dep is not None and dep.is_package: + nested, transitive = _resolve_package_cleanup(dep, config, existing_lockfile) + removed = _uninstall_transitive_entries( + transitive, config, tools, repo_root, skills_dirs + ) + return removed, nested, transitive + removed = _remove_leaf_from_filesystem( + dep, handle, config, tools, repo_root, skills_dirs + ) + return removed, [], [] + + +def _remove_from_config(config: AgrConfig, candidates: list[str]) -> bool: + """Remove the first matching candidate identifier from the config.""" + for identifier in candidates: + if config.remove_dependency(identifier): + return True + return False + + +def _build_removal( + ref: str, + dep: Dependency | None, + candidates: list[str], + nested_package_entries: list[tuple[str, LockedEntry]], + transitive_entries: list[tuple[str, LockedEntry]], +) -> _RefRemoval: + """Assemble a successful ``_RefRemoval`` with the lockfile entries to drop.""" + dep_kind = dep.type if dep is not None else DEPENDENCY_TYPE_SKILL + removal = _RefRemoval( + CommandResult(ref, True, "Removed"), + removed_candidates=[candidates], + removed_kinds=[dep_kind], + ) + for kind, entry in (*nested_package_entries, *transitive_entries): + removal.removed_candidates.append([entry.identifier]) + removal.removed_kinds.append(kind) + return removal + + +def _process_ref( + ref: str, + config: AgrConfig, + tools: list[ToolConfig], + repo_root: Path | None, + skills_dirs: dict[str, Path] | None, + existing_lockfile: Lockfile | None, + global_install: bool, +) -> _RefRemoval: + """Process a single remove ref: resolve, uninstall, and report the outcome. + + Performs filesystem removal and config removal for one ref (plus any + transitive/nested package children), returning a ``_RefRemoval`` whose + parallel candidate/kind lists feed ``_update_lockfile_after_remove``. + Install errors are caught and surfaced as a failed ``CommandResult``. + """ + try: + handle, candidates, dep = _resolve_dep(ref, config, global_install) + removed_fs, nested, transitive = _remove_from_filesystem( + dep, handle, config, tools, repo_root, skills_dirs, existing_lockfile + ) + removed_config = _remove_from_config(config, candidates) + if not (removed_fs or removed_config): + return _RefRemoval(CommandResult(ref, False, "Not found")) + return _build_removal(ref, dep, candidates, nested, transitive) + except INSTALL_ERROR_TYPES as e: + return _RefRemoval(CommandResult(ref, False, format_install_error(e))) + + def run_remove(refs: list[str], global_install: bool = False) -> None: """Run the remove command. @@ -225,113 +468,18 @@ def run_remove(refs: list[str], global_install: bool = False) -> None: removed_kinds: list[str] = [] for ref in refs: - try: - # Parse handle - handle = parse_handle(ref, default_owner=config.default_owner) - - # Compute the resolved absolute path once for local global installs - abs_path_str: str | None = None - if global_install and handle.is_local and handle.local_path is not None: - abs_path_str = str(handle.resolve_local_path()) - - candidates = _identifier_candidates(ref, handle, abs_path_str) - - dep = _find_dep_by_candidates(candidates, ref, config.dependencies) - - # When the dep was found by installed_name fallback (not by - # any candidate identifier), its actual identifier must be - # added to candidates so that config and lockfile removal - # can match it. - if dep is not None and dep.identifier not in candidates: - candidates.append(dep.identifier) - - source_name = None - if dep and dep.is_remote: - source_name = dep.source or config.default_source - - is_ralph = dep is not None and dep.is_ralph - dep_kind = dep.type if dep is not None else DEPENDENCY_TYPE_SKILL - fs_handle = dep.to_parsed_handle(config.default_owner) if dep else handle - - # Remove from filesystem - removed_fs = False - if dep is None or not dep.is_package: - removed_fs = _uninstall_from_filesystem( - fs_handle, - is_ralph, - tools, - repo_root, - source_name, - skills_dirs, - default_repo=config.default_repo, - ) - - transitive_entries: list[tuple[str, LockedEntry]] = [] - nested_package_entries: list[tuple[str, LockedEntry]] = [] - if dep is not None and dep.is_package: - direct_leaf_ids = { - (d.type, d.identifier) - for d in config.dependencies - if not d.is_package and d.identifier != dep.identifier - } - candidate_package_ids = ( - existing_lockfile.package_closure({dep.identifier}) - if existing_lockfile - else {dep.identifier} - ) - nested_package_entries = [ - (kind, entry) - for kind, entry in _nested_package_entries_for_packages( - existing_lockfile, {dep.identifier} - ) - if not (entry.parent_ids - candidate_package_ids) - ] - removed_package_ids = {dep.identifier} | { - entry.identifier for _kind, entry in nested_package_entries - } - transitive_entries = [ - (kind, entry) - for kind, entry in _transitive_leaf_entries_for_packages( - existing_lockfile, {dep.identifier} - ) - if (kind, entry.identifier) not in direct_leaf_ids - and not (entry.parent_ids - removed_package_ids) - ] - for kind, entry in transitive_entries: - child_handle = _entry_to_handle(entry, kind, config.default_owner) - if _uninstall_from_filesystem( - child_handle, - kind == DEPENDENCY_TYPE_RALPH, - tools, - repo_root, - entry.source, - skills_dirs, - default_repo=config.default_repo, - ): - removed_fs = True - - # Remove from config (try same candidate identifiers) - removed_config = False - for identifier in candidates: - if config.remove_dependency(identifier): - removed_config = True - break - - if removed_fs or removed_config: - results.append(CommandResult(ref, True, "Removed")) - removed_candidates.append(candidates) - removed_kinds.append(dep_kind) - for kind, entry in nested_package_entries: - removed_candidates.append([entry.identifier]) - removed_kinds.append(kind) - for kind, entry in transitive_entries: - removed_candidates.append([entry.identifier]) - removed_kinds.append(kind) - else: - results.append(CommandResult(ref, False, "Not found")) - - except INSTALL_ERROR_TYPES as e: - results.append(CommandResult(ref, False, format_install_error(e))) + removal = _process_ref( + ref, + config, + tools, + repo_root, + skills_dirs, + existing_lockfile, + global_install, + ) + results.append(removal.result) + removed_candidates.extend(removal.removed_candidates) + removed_kinds.extend(removal.removed_kinds) save_and_summarize_results( results, diff --git a/docs/diary/2026-06-09-radon-complexity-reduction.md b/docs/diary/2026-06-09-radon-complexity-reduction.md new file mode 100644 index 0000000..1dde7b4 --- /dev/null +++ b/docs/diary/2026-06-09-radon-complexity-reduction.md @@ -0,0 +1,113 @@ +# Diary: Reduce `run_remove` cyclomatic complexity (radon-complexity-reduction, iteration 1) + +The `agr` codebase has a complexity-reduction task tracked under +`tasks/2026-06-09-radon-complexity-reduction/`. Radon flagged `run_remove` +(`/agr/commands/remove.py`) as the single worst outlier at **CC 36 (grade E)**. This +diary covers the first iteration: decomposing `run_remove` to grade ≤ B with no behavior +change, backed by seam-level tests. + +## Step 1: Decompose `run_remove` into focused helpers + +**Author:** main + +### Prompt Context + +**Verbatim prompt:** "Go wiht A. Use uvx radon to verify it before you impelment it" +(following `/suggest-next-iteration`, where option A was "Pin `run_remove` behavior, then +refactor it to ≤ B"). Later: "can we bump the grade up even higher or would that be a bad +idea" and "great, write a diary, make logic commits and make a pr". + +**Interpretation:** Implement iteration A from the task — refactor `run_remove` from its +CC 36 baseline down to grade ≤ B — verifying the baseline with `uvx radon` first, then +confirming the result. Then assess whether to push past grade B, write a diary, make +logical commits, and open a PR. + +**Inferred intent:** Knock out the worst, riskiest complexity hotspot first against a +calm baseline, without changing any CLI behavior, and keep `main` shippable. + +### What I did +Verified the baseline with `uvx radon cc agr/commands/remove.py -s` (confirmed +`run_remove` at E(36)), then extracted the per-`ref` loop body of `run_remove` into a set +of small helpers in `/agr/commands/remove.py`: + +- A `_RefRemoval` dataclass holding one ref's outcome (its `CommandResult` plus the + parallel `removed_candidates` / `removed_kinds` lists that feed + `_update_lockfile_after_remove`). +- `_resolve_dep` — parse the handle, build identifier candidates, and find the matching + dependency (including the installed-name fallback). +- `_remove_from_filesystem` + `_remove_leaf_from_filesystem` — filesystem removal, + fanning out to package children when the dep is a package. +- `_nested_packages_to_remove`, `_transitive_leaves_to_remove`, and the thin + `_resolve_package_cleanup` coordinator — the package child-resolution logic, split so + each stays ≤ B. +- `_uninstall_transitive_entries`, `_remove_from_config`, `_build_removal`, and the + `_process_ref` orchestrator wrapping the per-ref `try/except`. + +`run_remove` itself became a thin loop: collect `_process_ref` results, flatten the +candidate/kind lists, summarize, and call `_update_lockfile_after_remove`. Added +`TestResolvePackageCleanup` and `TestProcessRef` to `/tests/unit/test_remove.py` covering +the new seams (transitive leaf, nested package, shared-child retention, no-lockfile, +not-found, leaf removal, package transitive scheduling). + +### Why +`run_remove` was the biggest complexity liability in the codebase and the riskiest to +touch (interacting transitive/nested-package cleanup branches), so doing it first means +every later iteration works against a calmer baseline. Pure decomposition — moving code +into named seams — keeps behavior identical while making each branch independently +testable. + +### What worked +The supporting helpers already existed (`_uninstall_from_filesystem`, +`_transitive_leaf_entries_for_packages`, `_nested_package_entries_for_packages`, +`_entry_to_handle`), and `test_remove.py` already covered the hard integration cases, so +the refactor leaned on a strong existing safety net. Final radon shows `run_remove` at +**A(2)** with no function above grade B; full suite `uv run pytest` reports 1310 passed, +6 skipped; ruff and `ty check` clean. + +### What didn't work +My first extraction left `_process_ref` at **D(23)** and `_resolve_package_cleanup` at +**C(11))** — both still over the bar — because I'd only lifted the loop body wholesale +into one function. I split them further (filesystem/config/build seams for `_process_ref`, +and the nested/transitive split for the package cleanup) to land everything at ≤ B. + +One test failed on first run: + +``` +tests/unit/test_remove.py::TestProcessRef::test_not_found_returns_failure +- Not found ++ repo_root is required when skills_dir is not provided +``` + +I'd passed `repo_root=None` to `_process_ref` for the not-found case, but +`_uninstall_from_filesystem` raises (an `INSTALL_ERROR`, caught and surfaced as the +failure message) when `repo_root` is `None`. Passing the test's `tmp_path` as `repo_root` +fixed it and exercised the real "not found" path. + +### What I learned +The complexity in `run_remove` was almost entirely the package branch's interleaved +filtering — `direct_leaf_ids`, `candidate_package_ids`, and `removed_package_ids` gating +which children get removed. Separating "which nested packages" from "which transitive +leaves" (the latter depending on the former) is the natural seam and the reason the split +reads cleanly rather than feeling arbitrary. + +### What was tricky +Hitting grade B required two passes — naive extraction just relocates the branch count. +The judgment call was *where* to cut: the shared-child-retention rule (a leaf required by +another package must survive) lives in `_transitive_leaves_to_remove` and is the subtle +correctness point a reviewer should focus on. + +### What warrants review +Look at `_resolve_package_cleanup` and its two sub-helpers in `/agr/commands/remove.py`: +confirm the parent-chain filtering still retains children shared with another package +(covered by `TestResolvePackageCleanup::test_keeps_child_shared_with_other_package` and +the pre-existing `test_remove_package_keeps_shared_child_required_by_other_package`). +Also confirm `_process_ref` preserves per-ref error isolation via the `INSTALL_ERROR_TYPES` +wrapper. + +### Future work +We discussed pushing past grade B and decided against it: the remaining B(6–9) functions +(e.g. `_update_lockfile_after_remove`, `_identifier_candidates`) are cohesive, and forcing +them to A would scatter logic into over-decomposed indirection for no maintainability gain. +Next iterations of the task target the real outliers — `_install_package` (D21), +`detect_conflicts` (C19), the `config.py` round-trip hotspots, the `sync.py` split, and +finally the xenon CI gate. diff --git a/tasks/2026-06-09-radon-complexity-reduction/LOG.md b/tasks/2026-06-09-radon-complexity-reduction/LOG.md new file mode 100644 index 0000000..cc35f05 --- /dev/null +++ b/tasks/2026-06-09-radon-complexity-reduction/LOG.md @@ -0,0 +1,37 @@ +# Iteration Log — radon-complexity-reduction + +## Iteration 1 — Refactor `run_remove` to CC ≤ B (AC #1) + +**Shipped.** Decomposed the worst complexity outlier, `run_remove` +(`agr/commands/remove.py`), from **CC 36 (grade E)** to **CC 2 (A)** with no behavior +change. + +### What changed +- Extracted the per-`ref` loop body into focused helpers, all grade ≤ B: + - `_RefRemoval` dataclass — per-ref outcome (result + parallel candidate/kind lists). + - `_resolve_dep` — parse handle, build identifier candidates, find matching dep. + - `_remove_leaf_from_filesystem` / `_remove_from_filesystem` — filesystem removal, + fanning out to package children. + - `_nested_packages_to_remove` / `_transitive_leaves_to_remove` / `_resolve_package_cleanup` + — package child resolution (split so each stays ≤ B). + - `_uninstall_transitive_entries`, `_remove_from_config`, `_build_removal`, `_process_ref`. +- `run_remove` is now a thin loop: collect `_process_ref` results → flatten → summarize → + `_update_lockfile_after_remove`. +- Reused all pre-existing helpers; no logic changes. + +### Tests +- Added `TestResolvePackageCleanup` (transitive leaf, nested package, shared-child + retention, no-lockfile) and `TestProcessRef` (not-found, leaf removal, package + transitive scheduling) to `tests/unit/test_remove.py`. +- Existing `run_remove` integration tests retained as the behavior-preservation net. + +### Verification +- `uvx radon cc agr/commands/remove.py -s`: no function above grade B (largest now B 9, + `_update_lockfile_after_remove`, pre-existing). +- `uv run pytest`: 1310 passed, 6 skipped. +- `uv run ruff check` / `ruff format --check` / `uv run ty check`: clean. +- Smoke: `agr --help`, `agr remove --help` OK. + +### Next up +AC #4 + #5 (package.py / config.py hotspots) is the natural next slice; sync.py split +(#3) and xenon gate (#7) remain last per staged-risk ordering. diff --git a/tests/unit/test_remove.py b/tests/unit/test_remove.py index 0b10819..024b975 100644 --- a/tests/unit/test_remove.py +++ b/tests/unit/test_remove.py @@ -8,6 +8,8 @@ from agr.commands.remove import ( _identifier_candidates, _find_dep_by_candidates, + _process_ref, + _resolve_package_cleanup, run_remove, ) from agr.config import AgrConfig, Dependency @@ -431,3 +433,140 @@ def test_remove_package_keeps_shared_child_required_by_other_package( assert [entry.handle for entry in lockfile.packages] == ["alice/repo/bundle-b"] assert len(lockfile.skills) == 1 assert lockfile.skills[0].parent_ids == {"alice/repo/bundle-b"} + + +class TestResolvePackageCleanup: + """Tests for _resolve_package_cleanup() — package child resolution seam.""" + + def test_collects_transitive_leaf_children(self): + dep = Dependency(type="package", handle="alice/repo/bundle") + config = AgrConfig(dependencies=[dep]) + lockfile = Lockfile( + packages=[LockedEntry(handle="alice/repo/bundle", installed_name="bundle")], + skills=[ + LockedEntry( + handle="alice/repo/child", + installed_name="child", + parent="alice/repo/bundle", + ) + ], + ) + + nested, transitive = _resolve_package_cleanup(dep, config, lockfile) + + assert nested == [] + assert [entry.handle for _kind, entry in transitive] == ["alice/repo/child"] + + def test_collects_nested_package_entries(self): + dep = Dependency(type="package", handle="alice/repo/top") + config = AgrConfig(dependencies=[dep]) + lockfile = Lockfile( + packages=[ + LockedEntry(handle="alice/repo/top", installed_name="top"), + LockedEntry( + handle="alice/repo/nested", + installed_name="nested", + parent="alice/repo/top", + ), + ], + ) + + nested, transitive = _resolve_package_cleanup(dep, config, lockfile) + + assert [entry.handle for _kind, entry in nested] == ["alice/repo/nested"] + assert transitive == [] + + def test_keeps_child_shared_with_other_package(self): + dep = Dependency(type="package", handle="alice/repo/bundle-a") + config = AgrConfig( + dependencies=[ + dep, + Dependency(type="package", handle="alice/repo/bundle-b"), + ] + ) + lockfile = Lockfile( + packages=[ + LockedEntry(handle="alice/repo/bundle-a", installed_name="bundle-a"), + LockedEntry(handle="alice/repo/bundle-b", installed_name="bundle-b"), + ], + skills=[ + LockedEntry( + handle="alice/repo/shared", + installed_name="shared", + parents=["alice/repo/bundle-a", "alice/repo/bundle-b"], + ) + ], + ) + + nested, transitive = _resolve_package_cleanup(dep, config, lockfile) + + # Shared child still required by bundle-b is NOT scheduled for removal. + assert nested == [] + assert transitive == [] + + def test_no_lockfile_returns_empty(self): + dep = Dependency(type="package", handle="alice/repo/bundle") + config = AgrConfig(dependencies=[dep]) + + nested, transitive = _resolve_package_cleanup(dep, config, None) + + assert nested == [] + assert transitive == [] + + +class TestProcessRef: + """Tests for _process_ref() — per-ref orchestration seam.""" + + def test_not_found_returns_failure(self, tmp_path): + config = AgrConfig(dependencies=[]) + + removal = _process_ref( + "alice/repo/missing", config, [CLAUDE], tmp_path, None, None, False + ) + + assert removal.result.success is False + assert removal.result.message == "Not found" + assert removal.removed_candidates == [] + assert removal.removed_kinds == [] + + def test_leaf_removal_drops_from_config_and_reports_candidate(self, tmp_path): + dep = Dependency(type="skill", handle="alice/repo/widget") + config = AgrConfig(dependencies=[dep]) + + # No filesystem footprint; config removal alone makes it a success. + with patch( + "agr.commands.remove._uninstall_from_filesystem", return_value=False + ): + removal = _process_ref( + "alice/repo/widget", config, [CLAUDE], tmp_path, None, None, False + ) + + assert removal.result.success is True + assert removal.removed_kinds == ["skill"] + assert config.dependencies == [] + + def test_package_removal_schedules_transitive_child(self, tmp_path): + dep = Dependency(type="package", handle="alice/repo/bundle") + config = AgrConfig(dependencies=[dep]) + lockfile = Lockfile( + packages=[LockedEntry(handle="alice/repo/bundle", installed_name="bundle")], + skills=[ + LockedEntry( + handle="alice/repo/child", + installed_name="child", + parent="alice/repo/bundle", + ) + ], + ) + + with patch( + "agr.commands.remove._uninstall_from_filesystem", return_value=False + ): + removal = _process_ref( + "bundle", config, [CLAUDE], tmp_path, None, lockfile, False + ) + + assert removal.result.success is True + assert removal.removed_kinds[0] == "package" + # The transitive child is appended for lockfile cleanup. + assert ["alice/repo/child"] in removal.removed_candidates