diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 81e6538..60bb1e8 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -14,12 +14,20 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v4 + with: + fetch-depth: 0 - name: Set up Ruby uses: ruby/setup-ruby@v1 with: ruby-version: '3.3' - name: Validate skill format and links run: ruby scripts/validate-skills.rb + - name: Test changed-skill quality validation + run: ruby scripts/test-validate-skill-quality.rb + - name: Validate changed skill quality + env: + SKILL_QUALITY_BASE: ${{ github.event.pull_request.base.sha || github.event.before }} + run: ruby scripts/validate-skill-quality.rb --base "$SKILL_QUALITY_BASE" - name: Check tracked repository artifacts run: python3 scripts/check-artifacts.py - name: Validate Claude Code marketplace diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2f922a9..3fcaa6a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -28,9 +28,12 @@ Clone the repository and run the validator from its root: git clone https://github.com/magnus919/agent-skills.git cd agent-skills ruby scripts/validate-skills.rb +ruby scripts/validate-skill-quality.rb --base origin/main ``` -The same validation runs in GitHub Actions for pushes and pull requests. If a skill includes executable scripts or a package, run its documented checks as well and include the commands and results in your pull request. +The structural validator checks the whole repository. The quality validator checks only added, renamed, modified, or uncommitted `SKILL.md` files relative to the supplied base. Changed descriptions must begin with an imperative verb and define a negative boundary in the description or a `When not to use` section. Generic no-op instructions are reported as warnings. The same validation runs in GitHub Actions for pushes and pull requests. + +If a skill includes executable scripts or a package, run its documented checks as well and include the commands and results in your pull request. ## Deprecating a skill diff --git a/scripts/test-validate-skill-quality.rb b/scripts/test-validate-skill-quality.rb new file mode 100644 index 0000000..580be0e --- /dev/null +++ b/scripts/test-validate-skill-quality.rb @@ -0,0 +1,302 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +require "fileutils" +require "minitest/autorun" +require "open3" +require "stringio" +require "tmpdir" + +require_relative "validate-skill-quality" + +class ValidateSkillQualityTest < Minitest::Test + def test_valid_imperative_description_and_boundary_pass + with_skill( + "Use this skill to review Ruby code. Not for application deployment.", + "# Ruby review\n" + ) do |path| + assert_empty SkillQualityValidator.new.validate(path) + end + end + + def test_passive_description_fails + with_skill( + "This skill reviews Ruby code. Not for application deployment.", + "# Ruby review\n" + ) do |path| + errors = SkillQualityValidator.new.validate(path).select { |finding| finding.severity == :error } + assert_equal 1, errors.length + assert_includes errors.first.message, "imperative verb" + end + end + + def test_missing_negative_boundary_fails + with_skill("Review Ruby code for correctness.", "# Ruby review\n") do |path| + errors = SkillQualityValidator.new.validate(path).select { |finding| finding.severity == :error } + assert_equal 1, errors.length + assert_includes errors.first.message, "negative boundary" + end + end + + def test_when_not_to_use_heading_satisfies_boundary + with_skill( + "Review Ruby code for correctness.", + "# Ruby review\n\n## When not to use\n\nUse a deployment skill for releases.\n" + ) do |path| + assert_empty SkillQualityValidator.new.validate(path) + end + end + + def test_empty_when_not_to_use_heading_does_not_satisfy_boundary + with_skill("Review Ruby code for correctness.", "# Ruby review\n\n## When not to use\n") do |path| + errors = SkillQualityValidator.new.validate(path).select { |finding| finding.severity == :error } + assert_equal 1, errors.length + assert_includes errors.first.message, "negative boundary" + end + end + + def test_non_substantive_when_not_to_use_sections_do_not_satisfy_boundary + bodies = [ + "## When not to use\n\n---\n", + "## When not to use\n\nTODO\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\n\n", + "## When not to use\n\n- \n", + "## 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" + ] + + bodies.each do |body| + with_skill("Review Ruby code for correctness.", body) do |path| + errors = SkillQualityValidator.new.validate(path).select { |finding| finding.severity == :error } + assert_equal 1, errors.length, body + assert_includes errors.first.message, "negative boundary" + end + end + end + + def test_incidental_unlike_does_not_satisfy_boundary + with_skill("Review Ruby behavior unlike prior benchmarks.", "# Ruby review\n") do |path| + errors = SkillQualityValidator.new.validate(path).select { |finding| finding.severity == :error } + assert_equal 1, errors.length + assert_includes errors.first.message, "negative boundary" + end + end + + def test_boundary_phrase_without_content_does_not_satisfy_boundary + with_skill("Review Ruby code. Not for.", "# Ruby review\n") do |path| + errors = SkillQualityValidator.new.validate(path).select { |finding| finding.severity == :error } + assert_equal 1, errors.length + assert_includes errors.first.message, "negative boundary" + end + end + + def test_common_imperative_outside_issue_examples_is_recognized + with_skill("Automate repository maintenance. Not for deployments.", "# Automation\n") do |path| + assert_empty SkillQualityValidator.new.validate(path) + end + end + + def test_no_op_phrases_warn_with_lines_without_blocking + body = <<~MARKDOWN + # Ruby review + Write clear code. + Write clean code. + Follow + best practices. + Handle errors appropriately and handle errors gracefully. + Ensure high quality. + Make it easy to read. + Write maintainable code. + + ## When not to use + Use a deployment skill for releases. + MARKDOWN + + with_skill("Review Ruby code for correctness.", body) do |path| + findings = SkillQualityValidator.new.validate(path) + assert_empty findings.select { |finding| finding.severity == :error } + warnings = findings.select { |finding| finding.severity == :warning } + assert_equal 8, warnings.length + assert_equal [6, 7, 8, 10, 10, 11, 12, 13], warnings.map(&:line).sort + assert warnings.all? { |warning| warning.message.include?("possible no-op instruction") } + end + end + + def test_malformed_frontmatter_is_reported_without_crashing + Dir.mktmpdir do |directory| + path = File.join(directory, "SKILL.md") + File.write(path, "---\nname: [\ndescription: nope\n---\n# Broken\n") + findings = SkillQualityValidator.new.validate(path) + assert_equal 1, findings.length + assert_equal :error, findings.first.severity + assert_includes findings.first.message, "invalid YAML" + end + end + + def test_selector_finds_added_modified_renamed_and_untracked_skills + with_repo do |root| + write_skill(root, "unchanged/SKILL.md", "This legacy description is intentionally invalid.") + write_skill(root, "modified/SKILL.md", valid_description("modified")) + write_skill(root, "old-name/SKILL.md", valid_description("old name")) + commit_all(root, "baseline") + base = git(root, "rev-parse", "HEAD").strip + + write_skill(root, "added/SKILL.md", valid_description("added")) + commit_all(root, "add skill") + write_skill(root, "modified/SKILL.md", valid_description("modified again")) + git(root, "mv", "old-name/SKILL.md", "old-name/RENAMED.md") + FileUtils.mkdir_p(File.join(root, "renamed")) + 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")) + + assert_equal( + %w[SKILL.md added/SKILL.md modified/SKILL.md renamed/SKILL.md untracked/SKILL.md], + ChangedSkillSelector.new(root: root, base: base).paths + ) + end + end + + def test_runner_ignores_unchanged_legacy_skill + with_repo do |root| + write_skill(root, "legacy/SKILL.md", "This legacy description lacks both rules.") + commit_all(root, "baseline") + base = git(root, "rev-parse", "HEAD").strip + write_skill(root, "new/SKILL.md", valid_description("new")) + + stdout = StringIO.new + stderr = StringIO.new + status = SkillQualityCheck.new(root: root, base: base, output: stdout, error: stderr).run + + assert_equal 0, status + refute_includes stdout.string, "legacy/SKILL.md" + refute_includes stderr.string, "legacy/SKILL.md" + assert_includes stdout.string, "Quality-checked 1 changed skill(s): 0 error(s), 0 warning(s)." + end + end + + def test_runner_exits_successfully_for_warning_only_findings + with_repo do |root| + File.write(File.join(root, "placeholder"), "baseline\n") + commit_all(root, "baseline") + base = git(root, "rev-parse", "HEAD").strip + write_skill( + root, + "warning/SKILL.md", + valid_description("warning"), + "# Warning\n\nFollow best practices.\n\n## When not to use\n\nUse another skill.\n" + ) + + stdout = StringIO.new + stderr = StringIO.new + status = SkillQualityCheck.new(root: root, base: base, output: stdout, error: stderr).run + + assert_equal 0, status + assert_includes stdout.string, "WARNING warning/SKILL.md:" + assert_includes stdout.string, "0 error(s), 1 warning(s)" + assert_empty stderr.string + end + end + + def test_runner_exits_nonzero_for_blocking_findings + with_repo do |root| + File.write(File.join(root, "placeholder"), "baseline\n") + commit_all(root, "baseline") + base = git(root, "rev-parse", "HEAD").strip + write_skill(root, "invalid/SKILL.md", "This skill has no negative boundary.", "# Invalid\n") + + stdout = StringIO.new + stderr = StringIO.new + status = SkillQualityCheck.new(root: root, base: base, output: stdout, error: stderr).run + + assert_equal 1, status + assert_includes stderr.string, "imperative verb" + assert_includes stderr.string, "negative boundary" + assert_includes stderr.string, "2 error(s), 0 warning(s)" + end + end + + def test_all_zero_base_validates_all_tracked_skills + with_repo do |root| + write_skill(root, "tracked/SKILL.md", valid_description("tracked")) + commit_all(root, "initial") + + assert_equal( + ["tracked/SKILL.md"], + ChangedSkillSelector.new(root: root, base: "0" * 40).paths + ) + end + end + + def test_explicit_base_compares_endpoint_snapshots_after_history_rewrite + with_repo do |root| + write_skill(root, "rewritten/SKILL.md", "This legacy description is invalid.", "# Legacy\n") + commit_all(root, "common ancestor") + common = git(root, "rev-parse", "HEAD").strip + + write_skill(root, "rewritten/SKILL.md", valid_description("before rewrite")) + commit_all(root, "before force push") + before = git(root, "rev-parse", "HEAD").strip + git(root, "reset", "--hard", common) + + assert_equal( + ["rewritten/SKILL.md"], + ChangedSkillSelector.new(root: root, base: before).paths + ) + end + end + + private + + def valid_description(subject) + "Review #{subject} behavior. Not for deployment work." + end + + def with_skill(description, body) + Dir.mktmpdir do |directory| + path = File.join(directory, "SKILL.md") + File.write(path, skill_text(description, body)) + yield path + end + end + + def with_repo + Dir.mktmpdir do |directory| + git(directory, "init", "-b", "main") + git(directory, "config", "user.name", "Test User") + git(directory, "config", "user.email", "test@example.invalid") + yield directory + end + end + + def write_skill(root, relative, description, body = "# Test\n\n## When not to use\n\nUse another skill.\n") + path = File.join(root, relative) + FileUtils.mkdir_p(File.dirname(path)) + File.write(path, skill_text(description, body)) + end + + def skill_text(description, body) + <<~SKILL + --- + name: test-skill + description: #{description.inspect} + --- + #{body} + SKILL + end + + def commit_all(root, message) + git(root, "add", "-A") + git(root, "commit", "-m", message) + end + + def git(root, *arguments) + stdout, stderr, status = Open3.capture3("git", *arguments, chdir: root) + assert status.success?, "git #{arguments.join(' ')} failed: #{stderr}" + stdout + end +end diff --git a/scripts/validate-skill-quality.rb b/scripts/validate-skill-quality.rb new file mode 100644 index 0000000..0881903 --- /dev/null +++ b/scripts/validate-skill-quality.rb @@ -0,0 +1,266 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +require "optparse" +require "open3" +require "pathname" +require "set" +require "yaml" + +class SkillQualityValidator + IMPERATIVE_VERBS = %w[ + add administer analyze apply assess audit automate author backup browse build calculate + capture chain check clean compare configure connect control convert create debug define + deploy design diagnose discover document draft edit evaluate export extract fetch find + fix flash format generate guide identify implement import ingest inspect install interact + investigate load maintain make manage migrate model monitor operate optimize organize parse + plan play prepare process publish query read refactor release remove render repair research + resolve restore review reverse-engineer route run scan scaffold scrape search secure select + send set simulate start stop structure summarize sync teach test track train transform + translate troubleshoot update use validate verify visualize write + ].to_set.freeze + + DESCRIPTION_BOUNDARY_PATTERNS = [ + /\bdo not use(?: this skill)? for\s+[`*_"']*[[:alnum:]]/i, + /\bnot for\s+[`*_"']*[[:alnum:]]/i, + /\bwhen not to use\s+[`*_"']*[[:alnum:]]/i, + /(?:\A|[.!?;:]\s+)unlike\s+[`*_"']*[[:alnum:]]/i, + /(?:\A|[.!?;:]\s+)distinct from\s+[`*_"']*[[:alnum:]]/i + ].freeze + BOUNDARY_HEADING = /^\#{1,6}\s+When not to use\s*\#*\s*$/i + NO_OP_PATTERNS = { + "write clear or clean code" => /\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 + }.freeze + + Finding = Struct.new(:severity, :path, :line, :message) + + def validate(path, relative: path.to_s) + text = File.read(path) + match = text.match(/\A---\r?\n(.*?)\r?\n---\r?\n/m) + return [Finding.new(:error, relative, 1, "missing YAML frontmatter")] unless match + + begin + data = YAML.safe_load(match[1], permitted_classes: [], aliases: false) + rescue Psych::Exception => error + return [Finding.new(:error, relative, 1, "invalid YAML: #{error.message.lines.first.strip}")] + end + + unless data.is_a?(Hash) + return [Finding.new(:error, relative, 1, "frontmatter must be a mapping")] + end + + description = data["description"] + unless description.is_a?(String) && !description.strip.empty? + return [Finding.new(:error, relative, 1, "description must be a non-empty string")] + end + + body = text[match.end(0)..] || "" + findings = [] + first_word = description.sub(/\ADeprecated:\s*/i, "").strip[/\A[[:alpha:]]+(?:-[[:alpha:]]+)?/] + unless first_word && IMPERATIVE_VERBS.include?(first_word.downcase) + findings << Finding.new( + :error, + relative, + 1, + "description must start with a recognized imperative verb (found #{first_word.inspect})" + ) + end + + unless negative_boundary?(description, body) + findings << Finding.new( + :error, + relative, + 1, + "description or body must define a negative boundary (for example, a 'When not to use' section)" + ) + end + + body_start_line = text[0...match.end(0)].count("\n") + 1 + NO_OP_PATTERNS.each do |label, pattern| + body.to_enum(:scan, pattern).each do + match = Regexp.last_match + findings << Finding.new( + :warning, + relative, + body_start_line + body[0...match.begin(0)].count("\n"), + "possible no-op instruction: #{label}" + ) + end + end + + findings + rescue Errno::ENOENT => error + [Finding.new(:error, relative, 1, error.message)] + end + + private + + def negative_boundary?(description, body) + return true if DESCRIPTION_BOUNDARY_PATTERNS.any? { |pattern| description.match?(pattern) } + + lines = body.lines + lines.each_with_index.any? do |line, index| + next false unless line.strip.match?(BOUNDARY_HEADING) + + section = lines[(index + 1)..].take_while { |candidate| !candidate.match?(/^\s*\#{1,6}\s+/) } + substantive_boundary_section?(section) + end + end + + def substantive_boundary_section?(lines) + in_comment = false + fence = nil + + lines.any? do |candidate| + content = strip_markdown_container(candidate.strip) + + if in_comment + in_comment = false if content.include?("-->") + next false + end + + if content.start_with?("") + next false + end + + if fence + fence = nil if content.start_with?(fence) + next false + end + + if (marker = content[/\A(?:```|~~~)/]) + fence = marker + next false + end + + visible = content.gsub(//, "").strip + visible = visible.gsub(/\A[*_`~]+|[*_`~]+\z/, "").strip + 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 + end + end + + def strip_markdown_container(content) + loop do + stripped = content.sub(/\A>\s?/, "").sub(/\A(?:[-+*]|\d+[.)])\s+/, "").strip + return content if stripped == content + + content = stripped + end + end +end + +class ChangedSkillSelector + ZERO_SHA = /\A0+\z/ + SKILL_PATHSPEC = ":(glob)**/SKILL.md" + + def initialize(root:, base: nil) + @root = Pathname(root).expand_path + @base = base + end + + def paths + selected = Set.new + base = resolved_base + + if base == :all + selected.merge(git_paths("ls-files", "-z", "--", SKILL_PATHSPEC)) + else + selected.merge(git_paths("diff", "--name-only", "--diff-filter=ACMR", "-z", base, "HEAD", "--", SKILL_PATHSPEC)) + end + + selected.merge(git_paths("diff", "--name-only", "--diff-filter=ACMR", "-z", "--", SKILL_PATHSPEC)) + selected.merge(git_paths("diff", "--cached", "--name-only", "--diff-filter=ACMR", "-z", "--", SKILL_PATHSPEC)) + selected.merge(git_paths("ls-files", "--others", "--exclude-standard", "-z", "--", SKILL_PATHSPEC)) + + selected + .reject { |path| path.include?("agent-council/profiles/skills/") } + .select { |path| (@root / path).file? } + .sort + end + + private + + def resolved_base + requested = @base || ENV["SKILL_QUALITY_BASE"] + return :all if requested&.match?(ZERO_SHA) + + requested ||= "origin/main" + output, status = git_capture("rev-parse", "--verify", "#{requested}^{commit}") + unless status.success? && !output.strip.empty? + raise ArgumentError, + "cannot establish a quality-check base from #{requested.inspect}; pass --base REF" + end + + output.strip + end + + def git_paths(*arguments) + output, status = git_capture(*arguments) + raise "git #{arguments.first} failed" unless status.success? + + output.split("\0").reject(&:empty?) + end + + def git_capture(*arguments) + stdout, stderr, status = Open3.capture3("git", *arguments, chdir: @root.to_s) + warn stderr unless status.success? || stderr.empty? + [stdout, status] + end +end + +class SkillQualityCheck + def initialize(root:, base: nil, output: $stdout, error: $stderr) + @root = Pathname(root).expand_path + @base = base + @output = output + @error = error + end + + def run + paths = ChangedSkillSelector.new(root: @root, base: @base).paths + if paths.empty? + @output.puts "No changed SKILL.md files to quality-check." + return 0 + end + + validator = SkillQualityValidator.new + findings = paths.flat_map do |relative| + validator.validate(@root / relative, relative: relative) + end + + findings.sort_by { |finding| [finding.path, finding.line, finding.severity.to_s, finding.message] }.each do |finding| + stream = finding.severity == :error ? @error : @output + stream.puts "#{finding.severity.to_s.upcase} #{finding.path}:#{finding.line}: #{finding.message}" + end + + error_count = findings.count { |finding| finding.severity == :error } + warning_count = findings.count { |finding| finding.severity == :warning } + summary = "Quality-checked #{paths.length} changed skill(s): #{error_count} error(s), #{warning_count} warning(s)." + error_count.zero? ? @output.puts(summary) : @error.puts(summary) + error_count.zero? ? 0 : 1 + rescue ArgumentError, RuntimeError => error + @error.puts "Skill quality check failed: #{error.message}" + 1 + end +end + +if $PROGRAM_NAME == __FILE__ + options = {} + parser = OptionParser.new do |opts| + opts.banner = "Usage: ruby scripts/validate-skill-quality.rb [--base REF]" + opts.on("--base REF", "Compare committed SKILL.md changes with REF") { |ref| options[:base] = ref } + end + parser.parse! + + root = File.expand_path("..", __dir__) + exit SkillQualityCheck.new(root: root, base: options[:base]).run +end