Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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\")
"

Expand Down Expand Up @@ -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\")
"

Expand All @@ -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\")
"

Expand All @@ -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\")
"

Expand All @@ -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\")
"

Expand Down
6 changes: 5 additions & 1 deletion docs/baseline.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 12 additions & 0 deletions docs/custom-rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
7 changes: 6 additions & 1 deletion docs/plugins.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion src/skillsaw/baseline.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"})
Comment thread
stbenjam marked this conversation as resolved.


@dataclass
Expand Down
53 changes: 53 additions & 0 deletions src/skillsaw/linter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Comment thread
not-stbenjam marked this conversation as resolved.
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
Expand Down Expand Up @@ -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:
Comment thread
not-stbenjam marked this conversation as resolved.
Comment thread
not-stbenjam marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject canonical builtin custom-rule IDs

When a repository-local custom rule returns a canonical builtin ID such as claude-plugin-readme, this new guard only checks RULE_ALIASES and ADVISORY_RULE_IDS; _known_rule_ids already contains the builtins but custom loading never consults it, so the custom rule can still be appended under the builtin's identity and share its config, suppressions, and baseline fingerprints. Please mirror the plugin loader's existing-ID check here, or narrow the new docs that promise builtin collisions are skipped.

AGENTS.md reference: AGENTS.md:L27-L29

Useful? React with 👍 / 👎.

reason = (
f"'{rid}' is a legacy alias of builtin rule '{RULE_ALIASES[rid]}'"
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
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
Expand Down
2 changes: 1 addition & 1 deletion src/skillsaw/rules/builtin/agentskills/valid.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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
Expand Down
38 changes: 38 additions & 0 deletions tests/test_plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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

Comment thread
coderabbitai[bot] marked this conversation as resolved.

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)."""
Expand Down
112 changes: 112 additions & 0 deletions tests/test_rule_aliases.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"""

import json
from pathlib import Path

import pytest

Expand Down Expand Up @@ -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
Comment thread
coderabbitai[bot] marked this conversation as resolved.


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 ─────────────────────────────────────────────────


Expand Down