Skip to content

refactor: P038 Tier-2 code hygiene cleanup (docs_url dedup, stub helper, typing, etc.) #41

@Jamie-BitFlight

Description

@Jamie-BitFlight

Story

As a maintainer of the codebase, I want to p038 tier-2 code hygiene cleanup (docs_url dedup, stub helper, typing, etc.) so that the code is cleaner and more maintainable.

Description

Problem

The P038 migration (#38) landed the functional monolith-to-modular extraction successfully (14 rule series registered, CU/CX adapter gap resolved, completeness tests green). Three review passes (reuse, quality, efficiency + QG Phase 1 code review + CodeRabbit + Copilot) surfaced a set of non-blocking code-hygiene findings that are worth consolidating in a dedicated cleanup pass. None of these items break behavior or fail CI; they are duplication, stringly-typed fields, and missing architectural-spec follow-throughs that will accumulate rot if deferred indefinitely.

Observations — findings catalogued from the P038 reviews

1. _docs_url duplicated across series modules

Every new series module defines an identical _docs_url(code: str) -> str helper:

  • packages/skilllint/rules/sk_series.py
  • packages/skilllint/rules/hk_series.py
  • packages/skilllint/rules/lk_series.py
  • packages/skilllint/rules/pd_series.py
  • packages/skilllint/rules/pl_series.py
  • packages/skilllint/rules/nr_series.py
  • (and the same pattern in cu/cx_series.py)

A canonical generate_docs_url already exists at packages/skilllint/plugin_validator.py:953 and is used by pa_series.py via deferred import. Reuse the canonical helper (deferred import inside function bodies to match the circular-import workaround) instead of 6+ copies.

2. Stub boilerplate across registration-only series

~25 @skilllint_rule-decorated functions across HK/LK/NR/PD/PL/SL/TC/PR series bodies are identical boilerplate:

@skilllint_rule("XX###", severity=..., category=..., platforms=..., authority=...)
def check_xxNNN(frontmatter: dict[str, object], path: Path, file_type: str) -> list[ValidationIssue]:
    """## XX### — ..."""
    return []

A register_stub_rule(rule_id, severity, category, ...) helper in rule_registry.py would let each stub series collapse the function-level boilerplate into a single declarative registration per rule. The reviewers flagged this as acceptable-but-collapsible.

3. Stringly-typed category and platforms decorator arguments

@skilllint_rule(severity=..., category=..., platforms=...) uses free-form strings and string lists. No enum or Literal type. Inconsistency risk: category="skill" vs category="skills" would not be caught. Consider Literal or StrEnum types sourced from a single canonical module (possibly co-located with EXPECTED_SERIES).

4. _NAME_RE duplicated across 3 modules

  • packages/skilllint/rules/as_series.py:51 defines _MAX_NAME_LENGTH = 64 and a _NAME_RE pattern locally
  • packages/skilllint/rules/fm_series.py:47 defines the same
  • packages/skilllint/rules/sk_series.py correctly imports _spec_constants.MAX_NAME_LENGTH (after the MAX_SKILL_NAME_LENGTH fix in commit 69e5e77)

The fm/as_series modules were pre-P038 and kept using their local constants. Standardizing them to import from _spec_constants would complete the canonicalization pattern.

5. Missing tests/test_cu_cx_registry.py

Architect spec §8.3 specifies an integration test that asserts:

  • CU001 and CU002 are in RULE_REGISTRY after import skilllint.rules
  • Cursor adapter validate() returns list[dict] (unchanged external contract)
  • No raw-dict ValidationIssue construction remains in adapters/cursor/adapter.py

A codex regression test (test_codex_adapter_ignores_non_agents_md in test_adapters.py:176) was added in commit 8fffcb0 but the explicit CU/CX registry-presence integration test from the architect spec is missing. These invariants are indirectly covered by test_rules_completeness.py but the explicit guard was spec'd and not delivered.

6. Pydantic RuleEntry at import time vs @dataclass(slots=True)

packages/skilllint/rule_registry.py:107-117 builds a Pydantic RuleAuthority + RuleEntry model at import time for each @skilllint_rule. With 58 decorators, that is 58 Pydantic validation passes during package import (the CLI cold-start path) plus duplicated docstring strings retained in memory. A @dataclass(slots=True, frozen=True) would reduce import-time cost and memory (rule registration is internal, no untrusted input).

7. Narrative comments in migrated series modules

Quality review flagged narrative comments explaining WHAT the code does (well-named identifiers already do that), or comments referencing the task/caller ("T3 replaces this", "Populated by T8"). Per CLAUDE.md comments should explain non-obvious WHY, not WHAT. A pass through the new series modules to remove narrative comments is ~20 minutes of work.

8. assert_rules_completeness.py script and test_rules_completeness.py::test_cli_rules_output_matches_registry are redundant

Both parse skilllint rules output to count distinct series prefixes. The test uses Typer's in-process CliRunner (cheap). The script uses subprocess (hundreds of ms cold-start). Both are wired into CI. Dropping one (and adding a skilllint rules --series-only or --format=json output flag for the script consumer) would be cleaner.

9. RuleEntry.fn is typed as Any

packages/skilllint/rule_registry.py:51 — the comment acknowledges "Callable — not validatable by Pydantic, stored as Any". Acceptable at this boundary but should be narrowed to Callable[..., list[ValidationIssue]] using a Protocol in a follow-on task.

User context

None — entirely internal cleanup. All findings are from machine-generated reviews during the P038 session. No user-reported bug.

Research questions

  • Should the @register_stub_rule helper live in rule_registry.py as a public API, or in a private _stub.py helper module?
  • For stringly-typed category / platforms, is Literal[...] sufficient or does a StrEnum add too much import-time cost?
  • Is the Pydantic-to-dataclass migration for RuleEntry worth the benchmark benefit? Worth measuring first.

Suggested location

Multiple: packages/skilllint/rule_registry.py, packages/skilllint/rules/{sk,lk,pd,pl,hk,nr,sl,tc,pr,cu,cx,as,fm}_series.py, packages/skilllint/tests/test_cu_cx_registry.py (new), scripts/assert_rules_completeness.py.

Scope boundary

This is a single-session cleanup pass. Each item is ~5-30 minutes. Can be batched into one PR or spread across several. Do NOT include behavior changes; the goal is pure hygiene.

Acceptance Criteria

  • Work matches description
  • Plan or implementation complete

Context

  • Source: P038 reviews (reuse + quality + efficiency + QG T1 code review + CodeRabbit + Copilot). Session 2026-04-13.
  • Priority: P2
  • Added: 2026-04-13
  • Research questions: None

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions