diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index af3bad1e..acd951b8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -217,7 +217,11 @@ jobs: assert r['stats']['plugins'] > 0, 'no plugins scanned' assert r['stats']['rules_run'] > 0, 'no rules ran' assert r['summary']['errors'] == 0, f\"unexpected errors: {r['summary']['errors']}\" - assert r['summary']['warnings'] == 0, f\"unexpected warnings: {r['summary']['warnings']}\" + # 'deprecated-rule' notices are advisory: they report that the linted + # repo's own skillsaw config names a deprecated rule, and they never + # affect skillsaw's exit code — so they must not fail this gate either. + warns = [v for v in r['violations'] if v['severity'] == 'warning' and v['rule_id'] != 'deprecated-rule'] + assert not warns, f\"unexpected warnings: {[(v['rule_id'], v['message']) for v in warns]}\" print(f\"Scanned {r['stats']['plugins']} plugins, {r['stats']['skills']} skills, {r['stats']['rules_run']} rules\") " @@ -253,7 +257,11 @@ jobs: r = json.load(sys.stdin) assert r['stats']['rules_run'] > 0, 'no rules ran' assert r['summary']['errors'] == 0, f\"unexpected errors: {r['summary']['errors']}\" - assert r['summary']['warnings'] == 0, f\"unexpected warnings: {r['summary']['warnings']}\" + # 'deprecated-rule' notices are advisory: they report that the linted + # repo's own skillsaw config names a deprecated rule, and they never + # affect skillsaw's exit code — so they must not fail this gate either. + warns = [v for v in r['violations'] if v['severity'] == 'warning' and v['rule_id'] != 'deprecated-rule'] + assert not warns, f\"unexpected warnings: {[(v['rule_id'], v['message']) for v in warns]}\" print(f\"Scanned {r['stats']['plugins']} plugins, {r['stats']['skills']} skills, {r['stats']['rules_run']} rules\") " @@ -266,7 +274,11 @@ jobs: r = json.load(sys.stdin) assert r['stats']['rules_run'] > 0, 'no rules ran' assert r['summary']['errors'] == 0, f\"unexpected errors: {r['summary']['errors']}\" - assert r['summary']['warnings'] == 0, f\"unexpected warnings: {r['summary']['warnings']}\" + # 'deprecated-rule' notices are advisory: they report that the linted + # repo's own skillsaw config names a deprecated rule, and they never + # affect skillsaw's exit code — so they must not fail this gate either. + warns = [v for v in r['violations'] if v['severity'] == 'warning' and v['rule_id'] != 'deprecated-rule'] + assert not warns, f\"unexpected warnings: {[(v['rule_id'], v['message']) for v in warns]}\" print(f\"Scanned {r['stats']['plugins']} plugins, {r['stats']['skills']} skills, {r['stats']['rules_run']} rules\") " @@ -279,7 +291,11 @@ jobs: r = json.load(sys.stdin) assert r['stats']['rules_run'] > 0, 'no rules ran' assert r['summary']['errors'] == 0, f\"unexpected errors: {r['summary']['errors']}\" - assert r['summary']['warnings'] == 0, f\"unexpected warnings: {r['summary']['warnings']}\" + # 'deprecated-rule' notices are advisory: they report that the linted + # repo's own skillsaw config names a deprecated rule, and they never + # affect skillsaw's exit code — so they must not fail this gate either. + warns = [v for v in r['violations'] if v['severity'] == 'warning' and v['rule_id'] != 'deprecated-rule'] + assert not warns, f\"unexpected warnings: {[(v['rule_id'], v['message']) for v in warns]}\" print(f\"Scanned {r['stats']['plugins']} plugins, {r['stats']['skills']} skills, {r['stats']['rules_run']} rules\") " @@ -292,7 +308,11 @@ jobs: r = json.load(sys.stdin) assert r['stats']['rules_run'] > 0, 'no rules ran' assert r['summary']['errors'] == 0, f\"unexpected errors: {r['summary']['errors']}\" - assert r['summary']['warnings'] == 0, f\"unexpected warnings: {r['summary']['warnings']}\" + # 'deprecated-rule' notices are advisory: they report that the linted + # repo's own skillsaw config names a deprecated rule, and they never + # affect skillsaw's exit code — so they must not fail this gate either. + warns = [v for v in r['violations'] if v['severity'] == 'warning' and v['rule_id'] != 'deprecated-rule'] + assert not warns, f\"unexpected warnings: {[(v['rule_id'], v['message']) for v in warns]}\" print(f\"Scanned {r['stats']['plugins']} plugins, {r['stats']['skills']} skills, {r['stats']['rules_run']} rules\") " diff --git a/docs/baseline.md b/docs/baseline.md index d9a8a4b9..6eafd3af 100644 --- a/docs/baseline.md +++ b/docs/baseline.md @@ -22,7 +22,11 @@ Once a `.skillsaw-baseline.json` file exists (next to `.skillsaw.yaml` or in the repo root), `skillsaw lint` automatically loads it and subtracts matching violations from the output. Only new violations are reported. Fatal infrastructure violations such as `repository-path-error` are not -written to the baseline and can never be suppressed by one. +written to the baseline and can never be suppressed by one. The same +goes for advisory `deprecated-rule` notices: baselining one would +permanently hide the warning that a rule is going away, so they are +never written and never suppressed — remove the deprecated rule from +your config to clear the notice instead. Violations are matched by a **content hash** — a fingerprint built from the rule ID, file path, and the content of the source line (not the line diff --git a/docs/custom-rules.md b/docs/custom-rules.md index f1ac4c30..59f739e3 100644 --- a/docs/custom-rules.md +++ b/docs/custom-rules.md @@ -111,6 +111,18 @@ rules: severity: warning ``` +### Rule IDs + +A custom rule's ID must not collide with a builtin, a legacy alias of a +renamed builtin (for example `plugin-readme`, now `claude-plugin-readme`), +or one of skillsaw's own advisory IDs (`deprecated-rule`). Aliases resolve +to the builtin everywhere a rule is named, so a custom rule using one could +never be configured or suppressed under its own ID; advisory IDs never +affect the exit code, so findings reported under one would not fail CI. A +rule claiming any of these is skipped with a `plugin-load-error` warning. +Prefix your IDs with something distinctive when in doubt +(`acme-no-todo`). + ### Key concepts | Concept | What the example shows | diff --git a/docs/plugins.md b/docs/plugins.md index 8333bfeb..64fa120e 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -242,7 +242,12 @@ class NoTodoInstructionsRule(Rule): Rule IDs must be unique across builtins and all installed plugins — a colliding plugin rule is skipped with a warning, never silently shadowed. -Prefix rule IDs with something distinctive when in doubt (`acme-no-todo`). +Legacy aliases of renamed builtins (for example `plugin-readme`, now +`claude-plugin-readme`) and skillsaw's own advisory IDs +(`deprecated-rule`) are reserved too: aliases resolve to the builtin +everywhere a rule is named, so a rule using one could never be +configured, and advisory IDs never affect the exit code. Prefix rule +IDs with something distinctive when in doubt (`acme-no-todo`). Plugins can also ship **deterministic autofixes** by setting `autofix_confidence` and overriding `fix()` — see the diff --git a/src/skillsaw/baseline.py b/src/skillsaw/baseline.py index f3cf28b6..9144a045 100644 --- a/src/skillsaw/baseline.py +++ b/src/skillsaw/baseline.py @@ -23,7 +23,10 @@ BASELINE_FILENAME = ".skillsaw-baseline.json" _BASELINE_VERSION = "1" -_UNBASELINABLE_RULE_IDS = frozenset({"repository-path-error"}) +# "deprecated-rule" mirrors linter.ADVISORY_RULE_IDS (kept literal to avoid +# a module cycle; pinned by a test): baselining a deprecation notice would +# permanently hide the removal warning it exists to deliver. +_UNBASELINABLE_RULE_IDS = frozenset({"repository-path-error", "deprecated-rule"}) @dataclass diff --git a/src/skillsaw/linter.py b/src/skillsaw/linter.py index 5c71aab3..ab48b25c 100644 --- a/src/skillsaw/linter.py +++ b/src/skillsaw/linter.py @@ -226,6 +226,32 @@ def _load_plugin_rules(self): ) continue + # Legacy aliases still resolve to their builtin everywhere a + # rule is named (config keys, flags, suppressions), so a + # plugin claiming one could never be addressed under its own + # name. Advisory IDs are reserved for skillsaw's own notices + # — a rule reporting under one would never affect the exit + # code. + from .rules.builtin import RULE_ALIASES + + if rid in RULE_ALIASES or rid in ADVISORY_RULE_IDS: + reason = ( + f"'{rid}' is a legacy alias of builtin rule '{RULE_ALIASES[rid]}'" + if rid in RULE_ALIASES + else f"'{rid}' is reserved for skillsaw's own advisory notices" + ) + self._plugin_load_violations.append( + RuleViolation( + rule_id="plugin-load-error", + severity=Severity.WARNING, + message=( + f"Plugin '{plugin.name}' provides rule '{rid}', but " + f"{reason} — the plugin's rule was skipped." + ), + ) + ) + continue + self._known_rule_ids.add(rid) if getattr(rule_instance, "deprecated", None) is not None: self._deprecated_known[rid] = rule_instance @@ -398,6 +424,33 @@ def _load_custom_rule(self, rule_path: str): and not inspect.isabstract(obj) ): rule_instance = obj() + + # Same reservation as plugin rules: a custom rule named + # after a legacy alias could never be addressed (config + # keys and flags resolve the alias to the builtin), and + # an advisory ID would exempt its findings from the exit + # code. + from .rules.builtin import RULE_ALIASES + + rid = rule_instance.rule_id + if rid in RULE_ALIASES or rid in ADVISORY_RULE_IDS: + reason = ( + f"'{rid}' is a legacy alias of builtin rule '{RULE_ALIASES[rid]}'" + if rid in RULE_ALIASES + else f"'{rid}' is reserved for skillsaw's own advisory notices" + ) + self._plugin_load_violations.append( + RuleViolation( + rule_id="plugin-load-error", + severity=Severity.WARNING, + message=( + f"Custom rule file {path.name} provides rule '{rid}', " + f"but {reason} — the rule was skipped." + ), + ) + ) + continue + self._known_rule_ids.add(rule_instance.rule_id) if getattr(rule_instance, "deprecated", None) is not None: self._deprecated_known[rule_instance.rule_id] = rule_instance diff --git a/src/skillsaw/rules/builtin/agentskills/valid.py b/src/skillsaw/rules/builtin/agentskills/valid.py index 88315b41..79cce9b7 100644 --- a/src/skillsaw/rules/builtin/agentskills/valid.py +++ b/src/skillsaw/rules/builtin/agentskills/valid.py @@ -84,7 +84,7 @@ def _plan_missing_frontmatter_fix(self, file_path: Path) -> Optional[Tuple[str, if original is None: return None kebab_name = _to_kebab(file_path.parent.name) - fixed = f"---\nname: {kebab_name}\ndescription: \n---\n{original}" + fixed = f"---\nname: {kebab_name}\ndescription:\n---\n{original}" new_fm, _new_body, new_error = parse_frontmatter(fixed) if new_error or not new_fm or new_fm.get("name") != kebab_name: return None diff --git a/tests/test_plugins.py b/tests/test_plugins.py index bd78eadd..71436dd7 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -83,6 +83,18 @@ def rule_id(self) -> str: return "plugin-deprecated-test" +class ShadowsAliasRule(AlwaysFiresRule): + @property + def rule_id(self) -> str: + return "plugin-readme" # legacy alias of claude-plugin-readme + + +class ShadowsAdvisoryRule(AlwaysFiresRule): + @property + def rule_id(self) -> str: + return "deprecated-rule" # reserved for skillsaw's advisory notices + + @pytest.fixture(autouse=True) def _clear_dist_fallback_cache(): plugins_mod._dist_by_entry_point.cache_clear() @@ -381,6 +393,32 @@ def test_rule_id_collision_with_builtin_is_skipped(fake_plugin, repo): assert warnings[0].severity == Severity.WARNING +def test_rule_id_collision_with_legacy_alias_is_skipped(fake_plugin, repo): + """A plugin rule claiming a legacy alias is skipped — config keys and + flags resolve the alias to the builtin, so the plugin's rule could + never be addressed under its own name.""" + fake_plugin("fake_alias_shadow", module_attrs={"SKILLSAW_RULES": [ShadowsAliasRule]}) + linter, violations = _lint(repo) + assert "plugin-readme" not in {r.rule_id for r in linter.rules} + warnings = [v for v in violations if v.rule_id == "plugin-load-error"] + assert len(warnings) == 1 + assert warnings[0].severity == Severity.WARNING + assert "legacy alias" in warnings[0].message + assert "claude-plugin-readme" in warnings[0].message + + +def test_rule_id_collision_with_advisory_id_is_skipped(fake_plugin, repo): + """A plugin rule claiming an advisory ID is skipped — its violations + would otherwise never affect the exit code.""" + fake_plugin("fake_advisory_shadow", module_attrs={"SKILLSAW_RULES": [ShadowsAdvisoryRule]}) + linter, violations = _lint(repo) + assert "deprecated-rule" not in {r.rule_id for r in linter.rules} + warnings = [v for v in violations if v.rule_id == "plugin-load-error"] + assert len(warnings) == 1 + assert warnings[0].severity == Severity.WARNING + assert "reserved" in warnings[0].message + + def test_deprecated_plugin_rule_inert_config_entry_warns(fake_plugin, repo): """A config entry naming a deprecated plugin rule warns even though the rule no longer runs (parity with builtin deprecation notices).""" diff --git a/tests/test_rule_aliases.py b/tests/test_rule_aliases.py index 03b8a4af..e4cad291 100644 --- a/tests/test_rule_aliases.py +++ b/tests/test_rule_aliases.py @@ -8,6 +8,7 @@ """ import json +from pathlib import Path import pytest @@ -193,6 +194,117 @@ def test_baseline_with_legacy_rule_id_still_suppresses(tmp_path): assert stale == [] +def test_advisory_ids_are_unbaselinable(): + """Baselining a deprecation notice would permanently hide the removal + warning; the literal in baseline.py must cover every advisory ID.""" + from skillsaw.baseline import _UNBASELINABLE_RULE_IDS, build_baseline + from skillsaw.linter import ADVISORY_RULE_IDS + + assert ADVISORY_RULE_IDS <= _UNBASELINABLE_RULE_IDS + + notice = RuleViolation( + rule_id="deprecated-rule", + severity=Severity.WARNING, + message="Rule 'x' is deprecated since 0.18.0", + ) + baseline = build_baseline([notice], Path("/tmp"), "0.18.0") + assert baseline.violations == [] + + +def test_baked_advisory_baseline_entry_does_not_suppress(tmp_path): + """An existing baseline may already contain an advisory entry; matching + one must not suppress the notice, or the removal warning stays hidden + until the baseline is regenerated.""" + notice = RuleViolation( + rule_id="deprecated-rule", + severity=Severity.WARNING, + message="Rule 'x' is deprecated since 0.18.0", + ) + baked = BaselineFile( + version="1", + generated_by="skillsaw 0.18.0", + generated_at="2026-01-01T00:00:00+00:00", + violations=[ + BaselineEntry( + fingerprint=fingerprint_violation(notice, tmp_path), + rule_id="deprecated-rule", + file_path=None, + line=None, + message=notice.message, + severity="warning", + ) + ], + root_path=tmp_path, + ) + kept, _stale = filter_baselined_violations([notice], baked, tmp_path) + assert kept == [notice] + + +def test_custom_rule_cannot_claim_legacy_alias(plugin_repo): + rule_file = plugin_repo / "lint_rule.py" + rule_file.write_text( + "from skillsaw.rule import Rule, Severity\n\n\n" + "class SquatterRule(Rule):\n" + " @property\n" + " def rule_id(self):\n" + ' return "plugin-readme"\n\n' + " @property\n" + " def description(self):\n" + ' return "claims a legacy alias"\n\n' + " def default_severity(self):\n" + " return Severity.ERROR\n\n" + " def check(self, context):\n" + ' return [self.violation("squatted")]\n' + ) + config = LinterConfig.default() + config.custom_rules = ["lint_rule.py"] + config.config_dir = plugin_repo + context = RepositoryContext(plugin_repo) + linter = Linter(context, config=config, no_plugins=True) + results = linter.run() + # The custom rule is skipped, so its violation never appears — and + # "plugin-readme" still means the builtin claude-plugin-readme wherever + # the name is used. + assert not any(v.message == "squatted" for v in results) + warnings = [v for v in results if v.rule_id == "plugin-load-error"] + assert len(warnings) == 1 + assert warnings[0].severity == Severity.WARNING + assert "legacy alias" in warnings[0].message + + +def test_custom_rule_cannot_claim_advisory_id(plugin_repo): + """The advisory half of the custom-rule reservation: findings reported + under an advisory ID are exempt from the exit code, so a custom rule + claiming one would report failures that never fail CI.""" + rule_file = plugin_repo / "lint_rule.py" + rule_file.write_text( + "from skillsaw.rule import Rule, Severity\n\n\n" + "class SquatterRule(Rule):\n" + " @property\n" + " def rule_id(self):\n" + ' return "deprecated-rule"\n\n' + " @property\n" + " def description(self):\n" + ' return "claims an advisory ID"\n\n' + " def default_severity(self):\n" + " return Severity.ERROR\n\n" + " def check(self, context):\n" + ' return [self.violation("squatted")]\n' + ) + config = LinterConfig.default() + config.custom_rules = ["lint_rule.py"] + config.config_dir = plugin_repo + context = RepositoryContext(plugin_repo) + linter = Linter(context, config=config, no_plugins=True) + results = linter.run() + assert "deprecated-rule" not in {r.rule_id for r in linter.rules} + assert not any(v.message == "squatted" for v in results) + warnings = [v for v in results if v.rule_id == "plugin-load-error"] + assert len(warnings) == 1 + assert warnings[0].severity == Severity.WARNING + assert "reserved" in warnings[0].message + + # ── Deprecation ─────────────────────────────────────────────────