mirror of
https://github.com/wshobson/agents
synced 2026-06-21 14:13:58 +00:00
fix(plugin-eval): exempt slash-only and path-triggered skills from MISSING_TRIGGER (#531)
* fix(plugin-eval): exempt slash-only and path-triggered skills from MISSING_TRIGGER Skills can be invoked through three mechanisms in Claude Code: 1. Model-driven auto-invocation based on the description (the default, and what MISSING_TRIGGER is designed to gate). 2. User-driven slash invocation only — opt-out via `disable-model-invocation: true` in the SKILL.md frontmatter. 3. Path-triggered auto-load — opt-in via the `paths:` frontmatter glob, where the skill is loaded when the model opens a matching file. The current `MISSING_TRIGGER` check runs against every skill regardless of invocation mechanism. Skills in (2) cannot be auto-invoked at all, so a trigger phrase in the description is irrelevant. Skills in (3) are triggered by file paths, not description matching, so the description serves as documentation rather than as a discovery surface. Add a `_skill_uses_description_trigger(skill)` predicate that consults `skill.frontmatter` and short-circuits the MISSING_TRIGGER check for the two non-description invocation paths. Behaviour for ordinary model-invocable skills is unchanged. Tests: - `test_disable_model_invocation_exempts_skill` — slash-only, no trigger, no flag. - `test_paths_auto_load_exempts_skill` — path-triggered, no trigger, no flag. - `test_disable_model_invocation_false_still_checks_trigger` — explicit `false` does not exempt. - `test_empty_paths_value_still_checks_trigger` — empty string is not a valid auto-load configuration. Full plugin-eval suite (73 tests) passes. * fix: address ruff SIM103/SIM102 and guard paths check against non-string values Simplify conditional returns (SIM103), collapse nested ifs (SIM102), and use isinstance(paths, str) to prevent paths: [] from incorrectly exempting a skill from MISSING_TRIGGER. --------- Co-authored-by: Seth Hobson <wshobson@gmail.com>
This commit is contained in:
@@ -32,6 +32,21 @@ def anti_pattern_penalty(count: int) -> float:
|
||||
return max(0.5, 1.0 - 0.05 * count)
|
||||
|
||||
|
||||
def _skill_uses_description_trigger(skill: ParsedSkill) -> bool:
|
||||
"""Return True if the skill relies on its description to be auto-invoked.
|
||||
|
||||
Skills that opt out of model-driven invocation (`disable-model-invocation:
|
||||
true`) or that auto-load on a path glob (`paths:` frontmatter) use a
|
||||
different trigger mechanism and should not be checked for a "Use when …"
|
||||
phrase in the description.
|
||||
"""
|
||||
fm = skill.frontmatter
|
||||
if fm.get("disable-model-invocation") is True:
|
||||
return False
|
||||
paths = fm.get("paths")
|
||||
return not (isinstance(paths, str) and paths != "")
|
||||
|
||||
|
||||
# Line count threshold for BLOATED_SKILL (no references/ dir)
|
||||
_BLOATED_LINE_THRESHOLD = 800
|
||||
|
||||
@@ -160,7 +175,14 @@ class StaticAnalyzer:
|
||||
# MISSING_TRIGGER: no recognised trigger phrasing in the description.
|
||||
# See `_TRIGGER_PATTERN` for the full list of accepted forms (imperative,
|
||||
# third-person canonical, prepositional, auto-load, etc.).
|
||||
if not _TRIGGER_PATTERN.search(skill.description):
|
||||
# Only applies to skills the model is expected to auto-invoke from the
|
||||
# description. Skills that are slash-only (`disable-model-invocation:
|
||||
# true`) or path-triggered (`paths:` frontmatter) use a different
|
||||
# invocation mechanism and should not be penalised for lacking a
|
||||
# description-level trigger phrase.
|
||||
if _skill_uses_description_trigger(skill) and not _TRIGGER_PATTERN.search(
|
||||
skill.description
|
||||
):
|
||||
patterns.append(
|
||||
AntiPattern(
|
||||
flag="MISSING_TRIGGER",
|
||||
|
||||
@@ -122,3 +122,86 @@ class TestTriggerPattern:
|
||||
result = analyzer.analyze_skill(skill_dir)
|
||||
flags = [ap.flag for ap in result.anti_patterns]
|
||||
assert "MISSING_TRIGGER" not in flags
|
||||
|
||||
|
||||
def _make_skill_with_frontmatter(
|
||||
tmp_path: Path, frontmatter_lines: list[str], name: str = "test-skill"
|
||||
) -> Path:
|
||||
skill_dir = tmp_path / name
|
||||
skill_dir.mkdir()
|
||||
frontmatter = "\n".join(frontmatter_lines)
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
f"---\n{frontmatter}\n---\n\n# Skill\n\n## Overview\n\nBody.\n"
|
||||
)
|
||||
return skill_dir
|
||||
|
||||
|
||||
class TestTriggerExemptions:
|
||||
"""`disable-model-invocation: true` and `paths:` frontmatter should exempt
|
||||
a skill from the MISSING_TRIGGER check, because those skills are not
|
||||
auto-invoked from the description.
|
||||
"""
|
||||
|
||||
def test_disable_model_invocation_exempts_skill(self, tmp_path: Path) -> None:
|
||||
skill_dir = _make_skill_with_frontmatter(
|
||||
tmp_path,
|
||||
[
|
||||
"name: setup",
|
||||
"description: One-time setup that adds .claude/state/ to the project's .gitignore.",
|
||||
"disable-model-invocation: true",
|
||||
],
|
||||
)
|
||||
analyzer = StaticAnalyzer()
|
||||
result = analyzer.analyze_skill(skill_dir)
|
||||
flags = [ap.flag for ap in result.anti_patterns]
|
||||
assert "MISSING_TRIGGER" not in flags, (
|
||||
"Slash-only skills should not be flagged for missing description trigger"
|
||||
)
|
||||
|
||||
def test_paths_auto_load_exempts_skill(self, tmp_path: Path) -> None:
|
||||
skill_dir = _make_skill_with_frontmatter(
|
||||
tmp_path,
|
||||
[
|
||||
"name: self-evaluate",
|
||||
"description: Self-critical evaluation guard for test/spec files.",
|
||||
'paths: "**/*test*,**/*spec*"',
|
||||
],
|
||||
)
|
||||
analyzer = StaticAnalyzer()
|
||||
result = analyzer.analyze_skill(skill_dir)
|
||||
flags = [ap.flag for ap in result.anti_patterns]
|
||||
assert "MISSING_TRIGGER" not in flags, (
|
||||
"Path-triggered skills should not be flagged for missing description trigger"
|
||||
)
|
||||
|
||||
def test_disable_model_invocation_false_still_checks_trigger(
|
||||
self, tmp_path: Path
|
||||
) -> None:
|
||||
skill_dir = _make_skill_with_frontmatter(
|
||||
tmp_path,
|
||||
[
|
||||
"name: model-invocable",
|
||||
"description: A description without a trigger phrase whatsoever.",
|
||||
"disable-model-invocation: false",
|
||||
],
|
||||
)
|
||||
analyzer = StaticAnalyzer()
|
||||
result = analyzer.analyze_skill(skill_dir)
|
||||
flags = [ap.flag for ap in result.anti_patterns]
|
||||
assert "MISSING_TRIGGER" in flags, (
|
||||
"Model-invocable skills without a trigger phrase must still be flagged"
|
||||
)
|
||||
|
||||
def test_empty_paths_value_still_checks_trigger(self, tmp_path: Path) -> None:
|
||||
skill_dir = _make_skill_with_frontmatter(
|
||||
tmp_path,
|
||||
[
|
||||
"name: bad-paths",
|
||||
"description: Some skill description that lacks the trigger phrase.",
|
||||
'paths: ""',
|
||||
],
|
||||
)
|
||||
analyzer = StaticAnalyzer()
|
||||
result = analyzer.analyze_skill(skill_dir)
|
||||
flags = [ap.flag for ap in result.anti_patterns]
|
||||
assert "MISSING_TRIGGER" in flags
|
||||
|
||||
Reference in New Issue
Block a user