Commit 5a998a1

mo khan <mo@mokhan.ca>
2026-09-08 05:01:00
refactor: split Skill from parsing, unify the summary index, tighten setup
- Skill is now a pure value Struct; SkillFile owns frontmatter/YAML parsing (I/O in .load, string parsing in .parse) so parse tests no longer need to touch disk. - Move truncation off Skill and onto Catalog#index(limit:), the single place that renders the joined summary list. The skill tool caps it (400 chars, since it ships on every provider.fetch); the /skills command does not, since a human reading it wants the full text. Previously these two renderers had silently drifted (tool truncated, command didn't cap consistently with tool). - Catalog#discover no longer smuggles its accumulator in as a default parameter; it's a local. Dropped the unused #each (no callers, no Enumerable include). - setup.rb is now a small Setup class (register_tool/register_command) instead of one long config.setup block.
1 parent 059df79
lib/elelem/skills/catalog.rb
@@ -20,19 +20,23 @@ module Elelem
         all.find { |skill| skill.name == name }
       end
 
-      def each(&block)
-        all.each(&block)
+      def index(limit: nil)
+        summaries = all.map(&:summary)
+        summaries = summaries.map { |text| truncate(text, limit) } if limit
+        summaries.join("\n")
       end
 
       private
 
-      def discover(seen = {})
+      def discover
+        seen = {}
+
         @load_paths.each do |path|
           dir = File.expand_path(path)
           next unless File.directory?(dir)
 
           Dir["#{dir}/*/SKILL.md"].sort.each do |skill_md|
-            skill = Skill.load(skill_md)
+            skill = SkillFile.load(skill_md)
             next unless skill
 
             seen[skill.name] = skill
@@ -43,6 +47,12 @@ module Elelem
 
         seen.values
       end
+
+      def truncate(text, limit)
+        return text if text.length <= limit
+
+        "#{text[0, limit]}..."
+      end
     end
   end
 end
lib/elelem/skills/setup.rb
@@ -1,44 +1,61 @@
 # frozen_string_literal: true
 
-Elelem.configure do |config|
-  config.setup(:skills) do |agent|
-    catalog = Elelem::Skills::Catalog.new
-    next if catalog.all.empty?
+module Elelem
+  module Skills
+    class Setup
+      SUMMARY_LIMIT = 400
 
-    index = -> do
-      catalog.all.map { |skill| Elelem::Skills::Skill.truncate(skill.summary) }.join("\n")
-    end
+      def self.call(agent)
+        new(agent, Catalog.new).call
+      end
 
-    agent.toolbox.add("skill",
-      description: "Load a skill's instructions by name. Available skills:\n#{index.call}",
-      params: { name: { type: "string", description: "the skill name" } },
-      required: ["name"]
-    ) do |a|
-      skill = catalog.find(a["name"])
-      next { error: "unknown skill: #{a["name"]}" } unless skill
+      def initialize(agent, catalog)
+        @agent = agent
+        @catalog = catalog
+      end
 
-      { name: skill.name, dir: skill.dir, content: skill.body }
-    end
+      def call
+        return if @catalog.all.empty?
+
+        register_tool
+        register_command
+      end
 
-    completions = -> { catalog.all.map(&:name) }
+      private
 
-    agent.commands.register("skills", description: "List available skills", completions: completions) do |arg|
-      if arg && !arg.empty?
-        skill = catalog.find(arg)
-        unless skill
-          agent.output.say "Unknown skill: #{arg}"
-          next
+      def register_tool
+        catalog = @catalog
+
+        @agent.toolbox.add("skill",
+          description: "Load a skill's instructions by name. Available skills:\n#{catalog.index(limit: SUMMARY_LIMIT)}",
+          params: { name: { type: "string", description: "the skill name" } },
+          required: ["name"]
+        ) do |a|
+          skill = catalog.find(a["name"])
+          next { error: "unknown skill: #{a["name"]}" } unless skill
+
+          { name: skill.name, dir: skill.dir, content: skill.body }
         end
+      end
+
+      def register_command
+        catalog = @catalog
+        agent = @agent
+        completions = -> { catalog.all.map(&:name) }
 
-        agent.output.say skill.body
-      else
-        list = catalog.all
-        if list.empty?
-          agent.output.say "No skills installed."
-        else
-          agent.output.say list.map(&:summary).join("\n")
+        agent.commands.register("skills", description: "List available skills", completions: completions) do |arg|
+          if arg && !arg.empty?
+            skill = catalog.find(arg)
+            skill ? agent.output.say(skill.body) : agent.output.say("Unknown skill: #{arg}")
+          else
+            agent.output.say(catalog.index)
+          end
         end
       end
     end
   end
 end
+
+Elelem.configure do |config|
+  config.setup(:skills) { |agent| Elelem::Skills::Setup.call(agent) }
+end
lib/elelem/skills/skill.rb
@@ -2,52 +2,7 @@
 
 module Elelem
   module Skills
-    class Skill
-      SUMMARY_LIMIT = 400
-
-      attr_reader :name, :description, :dir, :path, :body
-
-      def initialize(name:, description:, dir:, path:, body:)
-        @name = name
-        @description = description
-        @dir = dir
-        @path = path
-        @body = body
-      end
-
-      def self.truncate(text, limit: SUMMARY_LIMIT)
-        return text if text.length <= limit
-
-        "#{text[0, limit]}..."
-      end
-
-      def self.load(skill_md)
-        frontmatter, body = split(File.read(skill_md))
-        unless frontmatter
-          Elelem.logger.warn("elelem-skills: #{skill_md} has no frontmatter, skipping")
-          return nil
-        end
-
-        attrs = YAML.safe_load(frontmatter)
-        name, description = attrs["name"], attrs["description"]
-        unless name && description
-          Elelem.logger.warn("elelem-skills: #{skill_md} is missing name or description, skipping")
-          return nil
-        end
-
-        new(name: name, description: description, dir: File.dirname(skill_md), path: skill_md, body: body.strip)
-      end
-
-      def self.split(text)
-        return [nil, text] unless text.start_with?("---\n")
-
-        _, frontmatter, body = text.split(/^---\s*$/, 3)
-        return [nil, text] unless frontmatter && body
-
-        [frontmatter, body]
-      end
-      private_class_method :split
-
+    Skill = Struct.new(:name, :description, :dir, :body) do
       def summary
         "#{name}: #{description}"
       end
lib/elelem/skills/skill_file.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+module Elelem
+  module Skills
+    module SkillFile
+      def self.load(path)
+        parse(File.read(path), path: path)
+      end
+
+      def self.parse(text, path:)
+        frontmatter, body = split(text)
+        unless frontmatter
+          Elelem.logger.warn("elelem-skills: #{path} has no frontmatter, skipping")
+          return nil
+        end
+
+        attrs = YAML.safe_load(frontmatter)
+        name, description = attrs["name"], attrs["description"]
+        unless name && description
+          Elelem.logger.warn("elelem-skills: #{path} is missing name or description, skipping")
+          return nil
+        end
+
+        Skill.new(name, description, File.dirname(path), body.strip)
+      end
+
+      def self.split(text)
+        return [nil, text] unless text.start_with?("---\n")
+
+        _, frontmatter, body = text.split(/^---\s*$/, 3)
+        return [nil, text] unless frontmatter && body
+
+        [frontmatter, body]
+      end
+      private_class_method :split
+    end
+  end
+end
lib/elelem/skills.rb
@@ -5,5 +5,6 @@ require "yaml"
 
 require_relative "skills/version"
 require_relative "skills/skill"
+require_relative "skills/skill_file"
 require_relative "skills/catalog"
 require_relative "skills/setup"
spec/elelem/skills/catalog_spec.rb
@@ -42,4 +42,18 @@ RSpec.describe Elelem::Skills::Catalog do
       expect(catalog.find("nope")).to be_nil
     end
   end
+
+  describe "#index" do
+    it "joins summaries without truncating by default" do
+      catalog = described_class.new(load_paths: [fixtures])
+
+      expect(catalog.index).to eq("tdd: Test-driven development. Use when the user wants tests written first.")
+    end
+
+    it "truncates summaries when a limit is given" do
+      catalog = described_class.new(load_paths: [fixtures])
+
+      expect(catalog.index(limit: 10)).to eq("tdd: Test-...")
+    end
+  end
 end
spec/elelem/skills/skill_file_spec.rb
@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+RSpec.describe Elelem::Skills::SkillFile do
+  describe ".parse" do
+    it "parses frontmatter and body into a Skill" do
+      text = <<~SKILL
+        ---
+        name: tdd
+        description: Test-driven development.
+        ---
+
+        # TDD
+
+        Red, green, refactor.
+      SKILL
+
+      skill = described_class.parse(text, path: "/skills/tdd/SKILL.md")
+
+      expect(skill.name).to eq("tdd")
+      expect(skill.description).to eq("Test-driven development.")
+      expect(skill.dir).to eq("/skills/tdd")
+      expect(skill.body).to eq("# TDD\n\nRed, green, refactor.")
+    end
+
+    it "returns nil when description is missing" do
+      text = <<~SKILL
+        ---
+        name: tdd
+        ---
+        body
+      SKILL
+
+      expect(described_class.parse(text, path: "SKILL.md")).to be_nil
+    end
+
+    it "returns nil when there is no frontmatter" do
+      expect(described_class.parse("just a body, no frontmatter\n", path: "SKILL.md")).to be_nil
+    end
+  end
+
+  describe ".load" do
+    it "reads the file and parses it" do
+      fixtures = File.expand_path("../../fixtures/skills", __dir__)
+
+      skill = described_class.load(File.join(fixtures, "tdd", "SKILL.md"))
+
+      expect(skill.name).to eq("tdd")
+      expect(skill.dir).to eq(File.join(fixtures, "tdd"))
+    end
+
+    it "returns nil for a skill with no description" do
+      fixtures = File.expand_path("../../fixtures/skills", __dir__)
+
+      skill = described_class.load(File.join(fixtures, "no-description", "SKILL.md"))
+
+      expect(skill).to be_nil
+    end
+  end
+end
spec/elelem/skills/skill_spec.rb
@@ -1,53 +1,11 @@
 # frozen_string_literal: true
 
 RSpec.describe Elelem::Skills::Skill do
-  let(:fixtures) { File.expand_path("../../fixtures/skills", __dir__) }
-
-  describe ".load" do
-    it "parses frontmatter and body" do
-      skill = described_class.load(File.join(fixtures, "tdd", "SKILL.md"))
-
-      expect(skill.name).to eq("tdd")
-      expect(skill.description).to eq("Test-driven development. Use when the user wants tests written first.")
-      expect(skill.dir).to eq(File.join(fixtures, "tdd"))
-      expect(skill.body).to eq("# TDD\n\nRed, green, refactor.")
-    end
-
-    it "returns nil when description is missing" do
-      skill = described_class.load(File.join(fixtures, "no-description", "SKILL.md"))
-
-      expect(skill).to be_nil
-    end
-
-    it "returns nil when there is no frontmatter" do
-      Dir.mktmpdir do |dir|
-        path = File.join(dir, "SKILL.md")
-        File.write(path, "just a body, no frontmatter\n")
-
-        expect(described_class.load(path)).to be_nil
-      end
-    end
-  end
-
   describe "#summary" do
     it "combines name and description" do
-      skill = described_class.new(name: "tdd", description: "desc", dir: ".", path: "SKILL.md", body: "")
+      skill = described_class.new("tdd", "desc", ".", "")
 
       expect(skill.summary).to eq("tdd: desc")
     end
   end
-
-  describe ".truncate" do
-    it "leaves short text untouched" do
-      expect(described_class.truncate("short")).to eq("short")
-    end
-
-    it "caps long text and marks it as truncated" do
-      text = "a" * 500
-      result = described_class.truncate(text, limit: 400)
-
-      expect(result.length).to eq(403)
-      expect(result).to end_with("...")
-    end
-  end
 end