From dd457f1170065a7e84b78bf560f23732796e5e0f Mon Sep 17 00:00:00 2001 From: Magnus Hedemark Date: Tue, 21 Jul 2026 20:47:16 -0400 Subject: [PATCH] fix: close skill quality lint gaps (#98) Co-authored-by: magnus919 --- scripts/test-validate-skill-quality.rb | 58 +++++++++++++++++++++++++- scripts/validate-skill-quality.rb | 20 +++++---- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/scripts/test-validate-skill-quality.rb b/scripts/test-validate-skill-quality.rb index 580be0e..cdb9edc 100644 --- a/scripts/test-validate-skill-quality.rb +++ b/scripts/test-validate-skill-quality.rb @@ -66,7 +66,9 @@ class ValidateSkillQualityTest < Minitest::Test "## When not to use\n\n> \n", "## When not to use\n\n```text\nUse another skill instead.\n```\n", "## When not to use\n\n> ```text\n> Use another skill instead.\n> ```\n", - "## When not to use\n\n`TODO: Document this later.`\n" + "## When not to use\n\n`TODO: Document this later.`\n", + "## When not to use\n\nDetails will be added later.\n", + "## When not to use\n\nThis section explains deployment behavior.\n" ] bodies.each do |body| @@ -126,6 +128,56 @@ class ValidateSkillQualityTest < Minitest::Test end end + def test_inline_markdown_does_not_hide_no_op_phrases + body = <<~MARKDOWN + # Ruby review + Follow **best practices**. + Write *maintainable* code. + Follow __best practices__. + Write _maintainable_ code. + `Follow best practices.` + Call `follow_best_practices()`. + Call `follow__best__practices()`. + Follow ***best practices***. + Follow **_best practices_**. + Follow _**best practices**_. + Follow **best** *practices*. + Follow *best* **practices**. + Write **maintainable** **code**. + + ## When not to use + Use a deployment skill for releases. + MARKDOWN + + with_skill("Review Ruby code for correctness.", body) do |path| + warnings = SkillQualityValidator.new.validate(path).select { |finding| finding.severity == :warning } + assert_equal 11, warnings.length + assert_equal [6, 7, 8, 9, 10, 13, 14, 15, 16, 17, 18], warnings.map(&:line).sort + end + end + + def test_whole_phrase_emphasis_is_detected_for_every_no_op_family + phrases = [ + "Write clear code", + "Follow best practices", + "Handle errors gracefully", + "Ensure high quality", + "Make it easy to read", + "Write maintainable code" + ] + + phrases.each do |phrase| + ["**_#{phrase}_**.", "__#{phrase}__."].each do |formatted| + body = "#{formatted}\n\n## When not to use\nUse a deployment skill for releases.\n" + with_skill("Review Ruby code for correctness.", body) do |path| + warnings = SkillQualityValidator.new.validate(path).select { |finding| finding.severity == :warning } + assert_equal 1, warnings.length, formatted + assert_equal 5, warnings.first.line, formatted + end + end + end + end + def test_malformed_frontmatter_is_reported_without_crashing Dir.mktmpdir do |directory| path = File.join(directory, "SKILL.md") @@ -153,9 +205,11 @@ class ValidateSkillQualityTest < Minitest::Test git(root, "mv", "old-name/RENAMED.md", "renamed/SKILL.md") write_skill(root, "untracked/SKILL.md", valid_description("untracked")) write_skill(root, "SKILL.md", valid_description("root-level")) + write_skill(root, "agent-council/profiles/skills/vendored/SKILL.md", valid_description("vendored")) + write_skill(root, "not-agent-council/profiles/skills/changed/SKILL.md", valid_description("similarly named")) assert_equal( - %w[SKILL.md added/SKILL.md modified/SKILL.md renamed/SKILL.md untracked/SKILL.md], + %w[SKILL.md added/SKILL.md modified/SKILL.md not-agent-council/profiles/skills/changed/SKILL.md renamed/SKILL.md untracked/SKILL.md], ChangedSkillSelector.new(root: root, base: base).paths ) end diff --git a/scripts/validate-skill-quality.rb b/scripts/validate-skill-quality.rb index 0881903..f53f69c 100644 --- a/scripts/validate-skill-quality.rb +++ b/scripts/validate-skill-quality.rb @@ -28,13 +28,17 @@ class SkillQualityValidator /(?:\A|[.!?;:]\s+)distinct from\s+[`*_"']*[[:alnum:]]/i ].freeze BOUNDARY_HEADING = /^\#{1,6}\s+When not to use\s*\#*\s*$/i + BOUNDARY_CONTENT_PATTERN = /\b(?:avoid|belongs?|choose|defer|do not|don't|does not|doesn't|instead|never|no|not|only|outside|prefer|prerequisite|required?|requires?|rather than|route|see|skip|use|when)\b/i + MARKDOWN_LEAD = "(? /\bwrite\s+(?:clear|clean)\s+code\b/i, - "follow best practices" => /\bfollow\s+best\s+practices\b/i, - "handle errors appropriately or gracefully" => /\bhandle\s+errors\s+(?:appropriately|gracefully)\b/i, - "ensure high quality" => /\bensure\s+high\s+quality\b/i, - "make it easy to read" => /\bmake\s+it\s+easy\s+to\s+read\b/i, - "write maintainable code" => /\bwrite\s+maintainable\s+code\b/i + "write clear or clean code" => /#{MARKDOWN_LEAD}write#{MARKDOWN_GAP}(?:clear|clean)#{MARKDOWN_GAP}code#{MARKDOWN_TRAIL}\b/i, + "follow best practices" => /#{MARKDOWN_LEAD}follow#{MARKDOWN_GAP}best#{MARKDOWN_GAP}practices#{MARKDOWN_TRAIL}\b/i, + "handle errors appropriately or gracefully" => /#{MARKDOWN_LEAD}handle#{MARKDOWN_GAP}errors#{MARKDOWN_GAP}(?:appropriately|gracefully)#{MARKDOWN_TRAIL}\b/i, + "ensure high quality" => /#{MARKDOWN_LEAD}ensure#{MARKDOWN_GAP}high#{MARKDOWN_GAP}quality#{MARKDOWN_TRAIL}\b/i, + "make it easy to read" => /#{MARKDOWN_LEAD}make#{MARKDOWN_GAP}it#{MARKDOWN_GAP}easy#{MARKDOWN_GAP}to#{MARKDOWN_GAP}read#{MARKDOWN_TRAIL}\b/i, + "write maintainable code" => /#{MARKDOWN_LEAD}write#{MARKDOWN_GAP}maintainable#{MARKDOWN_GAP}code#{MARKDOWN_TRAIL}\b/i }.freeze Finding = Struct.new(:severity, :path, :line, :message) @@ -144,7 +148,7 @@ class SkillQualityValidator next false if visible.empty? || visible.match?(/\A[-*_]{3,}\z/) next false if visible.match?(/\A(?:TODO|TBD|TBA)\b/i) - visible.scan(/[[:alnum:]]+/).length >= 3 + visible.scan(/[[:alnum:]]+/).length >= 3 && visible.match?(BOUNDARY_CONTENT_PATTERN) end end @@ -182,7 +186,7 @@ class ChangedSkillSelector selected.merge(git_paths("ls-files", "--others", "--exclude-standard", "-z", "--", SKILL_PATHSPEC)) selected - .reject { |path| path.include?("agent-council/profiles/skills/") } + .reject { |path| path.start_with?("agent-council/profiles/skills/") } .select { |path| (@root / path).file? } .sort end