fix(skills): align skill catalog names with read_skill resolution

forge-cli/runtime/runner.go

@@ -3122,6 +3122,32 @@ func (r *Runner) buildSystemPrompt() string {
 // buildSkillCatalog generates a lightweight catalog of binary-backed skills
 // (those without scripts) for the system prompt. Script-backed skills are
 // already registered as first-class tools and don't need catalog entries.
+// skillEntryHasScript reports whether a `## Tool:` entry is backed by a
+// script that registerSkillTools actually registers as a first-class
+// callable tool — currently only `scripts/<name>.sh`. Such tools are
+// excluded from the read_skill catalog's "provides" list because the LLM
+// invokes them directly by name.
+//
+// This deliberately mirrors registerSkillTools' `.sh`-only lookup, NOT the
+// full set of script languages. A tool backed by a `.py`/`.js` script is
+// not yet registered as a callable tool, so it stays in "provides" (the
+// LLM reaches it by loading the skill), and read_skill's file listing
+// surfaces the actual script. Keep this in lockstep with
+// registerSkillTools: if it learns to register other script languages,
+// broaden this check at the same time or those tools vanish from both.
+func (r *Runner) skillEntryHasScript(skillDir, toolName string) bool {
+	scriptName := strings.ReplaceAll(toolName, "_", "-")
+	if skillDir != "" {
+		if _, err := os.Stat(filepath.Join(r.cfg.WorkDir, "skills", skillDir, "scripts", scriptName+".sh")); err == nil {
+			return true
+		}
+	}
+	if _, err := os.Stat(filepath.Join(r.cfg.WorkDir, "skills", "scripts", scriptName+".sh")); err == nil {
+		return true
+	}
+	return false
+}
+
 func (r *Runner) buildSkillCatalog() string {
 	matches := r.discoverSkillFiles()
 	if len(matches) == 0 {
@@ -3141,50 +3167,95 @@ func (r *Runner) buildSkillCatalog() string {
 			catalogSkillDir = filepath.Base(filepath.Dir(match))
 		}
 
-		// If no ## Tool: entries were parsed but frontmatter has name+description,
-		// create a synthetic entry so the skill appears in the catalog summary.
-		if len(entries) == 0 && meta != nil && meta.Name != "" && meta.Description != "" {
-			entries = []contract.SkillEntry{{
-				Name:        meta.Name,
-				Description: meta.Description,
-				Metadata:    meta,
-			}}
+		// The identifier advertised here MUST be the one the LLM passes
+		// to read_skill, which resolves a skill by its loadable name:
+		// the frontmatter `name` (also what the agent card advertises),
+		// falling back to the skill directory / flat-file name. Listing a
+		// `## Tool:` heading name here instead was the skill-lookup bug —
+		// read_skill("<tool>") 404'd whenever the tool name differed from
+		// the skill's name/directory (e.g. tool "k8s_triage" in skill
+		// "k8s-incident-triage"). One catalog line per skill, not per tool.
+		loadName := ""
+		switch {
+		case meta != nil && meta.Name != "":
+			loadName = meta.Name
+		case catalogSkillDir != "":
+			loadName = catalogSkillDir
+		default:
+			loadName = strings.TrimSuffix(filepath.Base(match), ".md")
 		}
 
-		for _, entry := range entries {
-			// Skip skills that have scripts (already registered as tools)
-			scriptName := strings.ReplaceAll(entry.Name, "_", "-")
-			hasScript := false
-			// Check subdirectory layout: skills/{dir}/scripts/{name}.sh
-			if catalogSkillDir != "" {
-				sp := filepath.Join(r.cfg.WorkDir, "skills", catalogSkillDir, "scripts", scriptName+".sh")
-				if _, err := os.Stat(sp); err == nil {
-					hasScript = true
+		// Description: prefer the frontmatter summary, else the first
+		// parsed tool entry that carries one.
+		desc := ""
+		if meta != nil {
+			desc = meta.Description
+		}
+		if desc == "" {
+			for _, e := range entries {
+				if e.Description != "" {
+					desc = e.Description
+					break
 				}
 			}
-			// Check legacy flat layout: skills/scripts/{name}.sh
-			if !hasScript {
-				sp := filepath.Join(r.cfg.WorkDir, "skills", "scripts", scriptName+".sh")
-				if _, err := os.Stat(sp); err == nil {
-					hasScript = true
+		}
+
+		if loadName == "" || desc == "" {
+			continue
+		}
+
+		// A `## Tool:` entry that is script- or binary-backed is registered
+		// as a first-class callable tool (see registerSkillTools) — the LLM
+		// invokes it directly by name and it must NOT appear here. Everything
+		// else is an instruction-only capability that the LLM reaches by
+		// loading this skill and following its steps. Collect those so the
+		// catalog surfaces WHAT the skill can do (tool selection) while the
+		// line's leading identifier stays the read_skill key (skill loading).
+		runtimeMode := contract.SkillRuntimeScript
+		if meta != nil && meta.Metadata != nil {
+			if forgeMap, ok := meta.Metadata["forge"]; ok {
+				if raw, ok := forgeMap["runtime"]; ok {
+					if s, ok := raw.(string); ok && s != "" {
+						runtimeMode = s
+					}
 				}
 			}
-			if hasScript {
+		}
+		var provides []string
+		usesCLI := false
+		for _, e := range entries {
+			if e.Name == "" {
 				continue
 			}
-
-			if entry.Name != "" && entry.Description != "" {
-				line := fmt.Sprintf("- %s: %s", entry.Name, entry.Description)
-				// Note that skill uses cli_execute without listing specific
-				// binary names — the LLM already sees the allowed enum in the
-				// tool schema, and listing names here leaks internal tooling
-				// when users ask "what skills/tools do you have?"
-				if entry.ForgeReqs != nil && len(entry.ForgeReqs.Bins) > 0 {
-					line += " (uses cli_execute)"
-				}
-				catalogEntries = append(catalogEntries, line)
+			if e.ForgeReqs != nil && len(e.ForgeReqs.Bins) > 0 {
+				usesCLI = true
+			}
+			if runtimeMode == contract.SkillRuntimeBinary || r.skillEntryHasScript(catalogSkillDir, e.Name) {
+				continue // registered as a callable tool; not a read_skill capability
 			}
+			provides = append(provides, e.Name)
+		}
+
+		// A fully script/binary-backed skill (has ## Tool: entries but none
+		// are instruction-only) is already exposed as callable tools — skip
+		// it. A pure-frontmatter skill (no ## Tool: entries) is an
+		// instruction skill and is kept.
+		if len(entries) > 0 && len(provides) == 0 {
+			continue
+		}
+
+		line := fmt.Sprintf("- %s: %s", loadName, desc)
+		if len(provides) > 0 {
+			line += fmt.Sprintf(" [provides: %s]", strings.Join(provides, ", "))
+		}
+		// Note that skill uses cli_execute without listing specific
+		// binary names — the LLM already sees the allowed enum in the
+		// tool schema, and listing names here leaks internal tooling
+		// when users ask "what skills/tools do you have?"
+		if usesCLI {
+			line += " (uses cli_execute)"
 		}
+		catalogEntries = append(catalogEntries, line)
 	}
 
 	if len(catalogEntries) == 0 {
@@ -3193,7 +3264,8 @@ func (r *Runner) buildSkillCatalog() string {
 
 	var b strings.Builder
 	b.WriteString("## Available Skills\n\n")
-	b.WriteString("Use `read_skill` to load full instructions for a skill before using it.\n\n")
+	b.WriteString("To use a skill, call `read_skill` with the skill name (the identifier before the colon) to load its full instructions, then follow them. " +
+		"`provides:` lists the capabilities inside a skill — they are documentation loaded with the skill, not tools you call directly.\n\n")
 	for _, entry := range catalogEntries {
 		b.WriteString(entry)
 		b.WriteString("\n")

forge-cli/runtime/runner_skill_catalog_test.go

@@ -0,0 +1,94 @@
+package runtime
+
+import (
+	"context"
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/initializ/forge/forge-core/tools/builtins"
+	"github.com/initializ/forge/forge-core/types"
+)
+
+// writeCatalogSkill creates skills/<dir>/SKILL.md with a frontmatter name
+// plus a `## Tool:` heading whose name differs from the skill name — the
+// exact shape that produced the read_skill lookup mismatch.
+func writeCatalogSkill(t *testing.T, root, dir, name, toolHeading string) {
+	t.Helper()
+	d := filepath.Join(root, "skills", dir)
+	if err := os.MkdirAll(d, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	content := "---\nname: " + name + "\ndescription: Read-only kubectl triage.\n---\n" +
+		"## Tool: " + toolHeading + "\n\nDoes the triage.\n"
+	if err := os.WriteFile(filepath.Join(d, "SKILL.md"), []byte(content), 0o644); err != nil {
+		t.Fatal(err)
+	}
+}
+
+// TestSkillCatalog_AdvertisesLoadableName asserts the catalog lists the
+// skill by its loadable (frontmatter) name, not the internal `## Tool:`
+// heading — the fix for the skill-lookup bug.
+func TestSkillCatalog_AdvertisesLoadableName(t *testing.T) {
+	root := t.TempDir()
+	writeCatalogSkill(t, root, "k8s-incident-triage", "k8s-incident-triage", "k8s_triage")
+
+	r := &Runner{cfg: RunnerConfig{WorkDir: root, Config: &types.ForgeConfig{AgentID: "test"}}}
+	cat := r.buildSkillCatalog()
+
+	// The read_skill key (leading identifier) must be the loadable name.
+	if !strings.Contains(cat, "- k8s-incident-triage:") {
+		t.Errorf("catalog should key the line on the loadable name; got:\n%s", cat)
+	}
+	// The tool heading must NOT be advertised as a loadable skill (the bug).
+	if strings.Contains(cat, "- k8s_triage:") {
+		t.Errorf("catalog must not key a line on the internal tool name; got:\n%s", cat)
+	}
+	// But the tool name IS surfaced as a capability for tool selection.
+	if !strings.Contains(cat, "provides: k8s_triage") {
+		t.Errorf("catalog should surface the skill's tool names under provides; got:\n%s", cat)
+	}
+}
+
+// TestSkillCatalog_NameIsReadSkillResolvable is the contract test that
+// would have caught the original bug: every name the catalog advertises
+// to the LLM must resolve through read_skill against the same workDir.
+func TestSkillCatalog_NameIsReadSkillResolvable(t *testing.T) {
+	root := t.TempDir()
+	// Two skills, both with a `## Tool:` heading that differs from the
+	// skill name, and one whose directory differs from its name.
+	writeCatalogSkill(t, root, "k8s-incident-triage", "k8s-incident-triage", "k8s_triage")
+	writeCatalogSkill(t, root, "kube", "cluster-inspector", "inspect")
+
+	r := &Runner{cfg: RunnerConfig{WorkDir: root, Config: &types.ForgeConfig{AgentID: "test"}}}
+	cat := r.buildSkillCatalog()
+
+	readTool := builtins.NewReadSkillTool(root)
+	var checked int
+	for _, line := range strings.Split(cat, "\n") {
+		if !strings.HasPrefix(line, "- ") {
+			continue
+		}
+		// Extract the advertised name: "- <name>: <desc>".
+		name := strings.TrimSpace(strings.SplitN(strings.TrimPrefix(line, "- "), ":", 2)[0])
+		if name == "" {
+			continue
+		}
+		out, err := readTool.Execute(context.Background(),
+			json.RawMessage(`{"name":`+mustQuote(name)+`}`))
+		if err != nil {
+			t.Fatalf("read_skill(%q): %v", name, err)
+		}
+		if strings.Contains(out, "not found") {
+			t.Errorf("catalog advertised %q but read_skill could not resolve it: %s", name, out)
+		}
+		checked++
+	}
+	if checked != 2 {
+		t.Fatalf("expected 2 advertised skills, checked %d\ncatalog:\n%s", checked, cat)
+	}
+}
+
+func mustQuote(s string) string { b, _ := json.Marshal(s); return string(b) }